Skip to content

[e2e-test] Add run bundle to e2e test#3831

Closed
varshaprasad96 wants to merge 2 commits intooperator-framework:masterfrom
varshaprasad96:run-bundle/e2e-test
Closed

[e2e-test] Add run bundle to e2e test#3831
varshaprasad96 wants to merge 2 commits intooperator-framework:masterfrom
varshaprasad96:run-bundle/e2e-test

Conversation

@varshaprasad96
Copy link
Copy Markdown
Member

@varshaprasad96 varshaprasad96 commented Sep 1, 2020

Description of the change:
Add run bundle to e2e operator test suite.

Motivation for the change:
Test run bundle integration with OLM.

Checklist

If the pull request includes user-facing changes, extra documentation is required:

@varshaprasad96
Copy link
Copy Markdown
Member Author

Ginko's skip works only for blocks within Its. Hence this needs to be on hold till run bundle is implemented, or could be commented and merged.
Note: Various run bundle scenarios will be tested in olm_integration_suite.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 3, 2020
Comment thread test/e2e/e2e_suite_test.go Outdated
Copy link
Copy Markdown
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment:

  • instead of use docker-push we load the image in the kind. Use LoadImageToKindClusterWithName(bundleImage)
  • The same need to be done for helm/ansible as well.

@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 14, 2020
Comment thread test/e2e/e2e_suite_test.go Outdated
Comment thread test/e2e/e2e_suite_test.go Outdated
Comment thread test/e2e/e2e_suite_test.go Outdated
Comment thread internal/cmd/operator-sdk/run/cmd.go
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 20, 2020
Comment thread test/e2e/e2e_suite_test.go Outdated
Comment thread test/e2e/e2e_suite_test.go Outdated
Comment thread test/e2e/e2e_suite_test.go Outdated
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2020
Comment thread test/e2e-go/e2e_go_olm_test.go
@varshaprasad96 varshaprasad96 force-pushed the run-bundle/e2e-test branch 4 times, most recently from 6846a96 to 3f25d7a Compare September 23, 2020 18:44
@@ -0,0 +1,4 @@
entries:
- description: >
Enable `run bundle` command and include e2e tests.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Enable `run bundle` command and include e2e tests.
Add the subcommand `operato-sdk run bundle` which allows users check their project integration with OLM via the bundle.

The e2e are not relevant to end-users

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @camilamacedo86 rephrase it. I would change her suggestion a bit:

Added the run bundle subcommand which allows users to verify their operator's bundle integrates with OLM.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be sufficient:

Suggested change
Enable `run bundle` command and include e2e tests.
Added the `run bundle` command.

@@ -34,7 +35,7 @@ Currently only the package manifests format is supported via the 'packagemanifes

cmd.AddCommand(
// TODO(joelanford): enable bundle command when implementation is complete
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// TODO(joelanford): enable bundle command when implementation is complete


By("destroying the Operator deployed with the 'run' subcommand")
cleanupPkgManCmd := exec.Command(tc.BinaryName, "cleanup", projectName,
"--timeout", "4m")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are running the bundle in a specific namespace. The ns that. is created for the operator (project-system).
Should not the cleanup be done with the ns as well?

Comment thread test/e2e-go/e2e_go_olm_test.go Outdated
}

By("running the operator bundle using `run bundle` command")
runBundleCmd := exec.Command(tc.BinaryName, "run", "bundle", bundleImage, "--namespace", tc.Kubectl.Namespace)
Copy link
Copy Markdown
Contributor

@camilamacedo86 camilamacedo86 Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The command is not working see:

Screen Shot 2020-09-24 at 07 31 19

See that the error lead to us to beleive that the image was NOT load in the Kind. So, the problem shows be in the isRunningOnKind. It shows has not the kubectx outside of the suite. See;

func isRunningOnKind() bool {
return strings.Contains(kubectx, "kind")
}
. You can debug the tests locally to check it.

Suggested solution:

// TestContext wraps kubebuilder's e2e TestContext.
type TestContext struct {
	*kbtestutils.TestContext

         // Kubectx stores the k8s context from where the tests are running
         Kubectx string
}
func (tc TestContext) IsRunningOnKind() bool {
      return strings.Contains(tc.Kubectx, "kind")
}
  • the if will be tc.IsRunningOnKind on the tests
  • In the suite code we will set the value: tc.Kubectx = the type

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, was trying to debug this error with Jesus. IsRunningOnKind works fine, but the run bundle logic always reaches to the remote registry to pull and unpack the image. Jesus is working on finding a way out for that. Meanwhile we decided to push the image to a remote repository, and check if it works. But there is some error in the install plan. Trying to figure that out.

Copy link
Copy Markdown
Contributor

@camilamacedo86 camilamacedo86 Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @varshaprasad96,

Since we use the IsRunningOnKind in all tests and in more than one place, IMO it should be centralized such as in order to be easy keep it maintained: #3941

Regards the issue, see that in the above pr;

STEP: loading the bundle image into Kind cluster
running: kind load docker-image e2e-test/controller-manager-bundle:mbnd --name kind
STEP: running the operator bundle using run bundle command
running: operator-sdk run bundle e2e-test/controller-manager-bundle:mbnd --namespace e2e-mbnd-system

Which means that the image was loaded in the Kind. So, the problem shows be a bug in the operator-sdk run bundle.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@camilamacedo86 yes, there is no error with IsRunningOnKind. The implementation logic of run bundle expects the image to be in registry. Even if its loaded in the kind cluster, while unpacking it tries to pull it from registry.

Copy link
Copy Markdown
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just minor changes to the fragment and etc.

@@ -0,0 +1,4 @@
entries:
- description: >
Enable `run bundle` command and include e2e tests.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @camilamacedo86 rephrase it. I would change her suggestion a bit:

Added the run bundle subcommand which allows users to verify their operator's bundle integrates with OLM.

@jmrodri jmrodri mentioned this pull request Sep 30, 2020
2 tasks
// bundle image should be present in the remote repository in run bundle
// implementation.
By("push the image to a remote repository")
err = tc.Make("docker-push", "IMG="+runBundleImg)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you're reverting this change @varshaprasad96?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@estroz no, afaik the image has to be pushed otherwise run bundle can't seem to pull the code.

@camilamacedo86 camilamacedo86 added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2020
@camilamacedo86
Copy link
Copy Markdown
Contributor

Hi @varshaprasad96,

It is outdated and we need to solve the issue with the run bundle to be able to do that?
Do you think is valid keep this PR open or track an issue and link this one for we have the history to know how we can do the new PR? So, if we move forward in this way, would be nice to have a link for the issue to solve the run bundle issue as well.

WDYT @jmrodri

@varshaprasad96
Copy link
Copy Markdown
Member Author

@camilamacedo86 this PR can be closed. These test cases would be covered in run bundle unit tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants