-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cross-component E2E for operator framework #286
Cross-component E2E for operator framework #286
Conversation
Codecov Report
@@ Coverage Diff @@
## main #286 +/- ##
=======================================
Coverage 82.84% 82.84%
=======================================
Files 21 21
Lines 892 892
=======================================
Hits 739 739
Misses 107 107
Partials 46 46
Flags with carried forward coverage won't be shown. Click here to find out more. |
6a5266d
to
d57d09e
Compare
b39378b
to
60dde4f
Compare
|
||
By("Verifying the operator doesn't exist") | ||
Eventually(func(g Gomega) { | ||
err = checkOperatorDeleted(catalogDInfo.operatorName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the func name checkOperatorDeleted(...)
, it might make more sense for it to return a bool and do something like return err != nil && errors.IsNotFound(err)
inside of the func, which would also simplify the checks here a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err
is returned, so that the reason for the failure of the test case would be shown if the assertion fails. This is the case for the other functions starting with check
as well. So wouldn't it be better to change the name to validateOperatorDeletion
instead of returning a bool ?
catalogDInfo.catalogDir = catalogDInfo.operatorName + "-catalog" | ||
catalogDInfo.imageRef = remoteRegistryRepo + catalogDInfo.catalogDir + ":test" | ||
}) | ||
When("Build and load plain+v0 bundle images into the test environment", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than directly committing plain bundle manifests, it should be possible to generate the manifests from an SDK project with kustomize build config/default > ${bundleRoot}/manifests/manifest.yaml
Firstly, this looks great! It's awesome to have that full coast-to-coast test that starts from scratch that we've been talking about. I looks like we cover the following steps (correct me if I'm wrong):
What I'm really interested in is which of these steps is more difficult than one or two shell commands, because that is where I think we should focus our efforts on UX to make things easier for our users. It might be nice to organize the test such that each of these steps is a function. If the function calls a single shell command that calls one of our tools, that's the ideal. If the function is a bunch of go code, that likely means that we have a tooling gap. |
a84335f
to
87fa9b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is super useful. I left a few comments/suggestions.
Most of them are minor. Major themes are:
- I didn't manage to run it locally: macos uses port 5000 + something is up with pushing images to the local registry
- If I understnad correctly,
It
specs depend on each other and it might be an issue when ran out of expected order. - There is quite a lot of code duplication in
test/operator-framework-e2e/operator_framework_test.go
. I'm wondering if we can reduce it.
opm: $(OPM) | ||
$(OPM) $(OPM_ARGS) | ||
|
||
operator-sdk: $(OPERATOR_SDK) | ||
(cd $(OPERATOR_SDK_PROJECT_PATH) && $(OPERATOR_SDK) $(OPERATOR_SDK_ARGS)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm missing something - we do not use these two targets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah... I see - we use them in tests. This is not obvous and I think deserves a comment.
However in my opinion E2E tests should not be calling make
to install opm
and operator-sdk
binaries. I think it is not critical for now, but we need to create an issue for this (unless there are objections to the idea below).
I think we need to change tests so that it:
- Either has
opm
andoperator-sdk
inPATH
and simply runs it likeexec.Command("opm", ...)
- Or receives a path to
opm
and friends via arguments/env vars or somehow else.
This way we can run a matrix for our tests and use different versions of opm
and operator-sdk
.
Also it will make test code a bit cleaner (but would require some more test setup code instead).
type Object struct { | ||
Kind string `yaml:"kind"` | ||
APIVersion string `yaml:"apiVersion"` | ||
Metadata struct { | ||
Name string `yaml:"name"` | ||
} `yaml:"metadata"` | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are looking for something like k8s.io/apimachinery/pkg/runtime/serializer
. Here is an example usage in our project:
operator-controller/cmd/resolutioncli/input_manifests.go
Lines 44 to 45 in fa84c73
decoder := codecs.UniversalDecoder(scheme.PrioritizedVersionsAllGroups()...) | |
object, _, err := decoder.Decode(fileContent, nil, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the code using the universal decoder. However, in cases where a single file contains multiple YAML documents separated by ---
, the universal decoder recognizes only the first resource. This situation is relevant as for plain+v0
bundle generated through kustomize
, the manifest has multiple YAML documents are combined into one file using ---
separators. Is there a decoder solution that can effectively handle scenarios involving multiple YAML documents within a single file? This is now handled by splitting the content of the YAML file and decoding each of them using the universal decoder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
I think YAMLToJSONDecoder
from k8s.io/apimachinery/pkg/util/yaml
should do the trick.
I think you should be able to get kind, api version and name if you decode into Unstructured
from k8s.io/apimachinery/pkg/apis/meta/v1/unstructured
.
cmd.Stderr = w | ||
cmd.Stdout = w | ||
if err := cmd.Run(); err != nil { | ||
return fmt.Errorf("Error pushing Docker container image: %s to the registry: %v", tag, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gettign this error:
Error pushing Docker container image: localhost:5001/plain-catalog:test to the registry: exit status 125
I use podman
with an alias from docker
to podman
.
I changed the local port from 5000 to 5001 due to issue with control center on macos mentioned above, but still fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test ran successfully on my macos. So I guess I am missing out something here. A local registry was used over the container registry quay.io
for improved performance. What could be an ideal solution for the issue with the port?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on version Ventura 13.4.1 right now. It looks like it started listening on port 5000 starting from Monterey.
What could be an ideal solution for the issue with the port?
I would change the localhost port to something else other than 5000. For example, kind suggests using port 5001. And it looks like kind changed from 5000 to 5001 precisely due to this reason:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the port to use 5001 on the host. But the port 5001 on the host maps to port 5000 on the container. This is because, the docker container uses the image registry:2
which listens on port 5000. This works for me. I am on version Ventura 13.4. Could you please confirm if the error is now resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the port 5001 on the host maps to port 5000 on the container.
@jubittajohn that's good. This is what we want. I just quickly chekced - registry is running fine now, but I'm still getting an error on an attempt to push the image:
[FAILED] in [It] - /Users/mradchuk/workspace/src/github.com/operator-framework/operator-controller/test/operator-framework-e2e/operator_framework_test.go:318 @ 08/09/23 10:13:38.056
<< Timeline
[FAILED] Unexpected error:
<*errors.errorString | 0x1400014f4f0>:
Error pushing bundle image localhost:5001/registry-operator-bundle:v0.1.0 with tag 0.1.0 : make[1]: Entering directory '/Users/mradchuk/workspace/src/github.com/operator-framework/operator-controller/test/operator-framework-e2e/registry-operator'
make docker-push IMG=localhost:5001/registry-operator-bundle:v0.1.0
make[2]: Entering directory '/Users/mradchuk/workspace/src/github.com/operator-framework/operator-controller/test/operator-framework-e2e/registry-operator'
docker push localhost:5001/registry-operator-bundle:v0.1.0
Getting image source signatures
Copying blob sha256:dea1f3bba92bc840b14e6ae4e75f6d1538be9c7d11b13e27dadb691cd16c6a04
Copying blob sha256:b6d433180825c5881b9565b9cbff353bace1926fe429c2cc4c67b75b203c8706
Copying blob sha256:7e13d0a7484e3297efc0eb92610081b75a115f7a24b2ffde5e25bde7f6619634
Error: trying to reuse blob sha256:b6d433180825c5881b9565b9cbff353bace1926fe429c2cc4c67b75b203c8706 at destination: pinging container registry localhost:5001: Get "https://localhost:5001/v2/": http: server gave HTTP response to HTTPS client
make[2]: *** [Makefile:131: docker-push] Error 125
make[2]: Leaving directory '/Users/mradchuk/workspace/src/github.com/operator-framework/operator-controller/test/operator-framework-e2e/registry-operator'
make[1]: *** [Makefile:240: bundle-push] Error 2
make[1]: Leaving directory '/Users/mradchuk/workspace/src/github.com/operator-framework/operator-controller/test/operator-framework-e2e/registry-operator'
: exit status 2
{
s: "Error pushing bundle image localhost:5001/registry-operator-bundle:v0.1.0 with tag 0.1.0 : make[1]: Entering directory '/Users/mradchuk/workspace/src/github.com/operator-framework/operator-controller/test/operator-framework-e2e/registry-operator'\nmake docker-push IMG=localhost:5001/registry-operator-bundle:v0.1.0\nmake[2]: Entering directory '/Users/mradchuk/workspace/src/github.com/operator-framework/operator-controller/test/operator-framework-e2e/registry-operator'\ndocker push localhost:5001/registry-operator-bundle:v0.1.0\nGetting image source signatures\nCopying blob sha256:dea1f3bba92bc840b14e6ae4e75f6d1538be9c7d11b13e27dadb691cd16c6a04\nCopying blob sha256:b6d433180825c5881b9565b9cbff353bace1926fe429c2cc4c67b75b203c8706\nCopying blob sha256:7e13d0a7484e3297efc0eb92610081b75a115f7a24b2ffde5e25bde7f6619634\nError: trying to reuse blob sha256:b6d433180825c5881b9565b9cbff353bace1926fe429c2cc4c67b75b203c8706 at destination: pinging container registry localhost:5001: Get \"https://localhost:5001/v2/\": http: server gave HTTP response to HTTPS client\nmake[2]: *** [Makefile:131: docker-push] Error 125\nmake[2]: Leaving directory '/Users/mradchuk/workspace/src/github.com/operator-framework/operator-controller/test/operator-framework-e2e/registry-operator'\nmake[1]: *** [Makefile:240: bundle-push] Error 2\nmake[1]: Leaving directory '/Users/mradchuk/workspace/src/github.com/operator-framework/operator-controller/test/operator-framework-e2e/registry-operator'\n: exit status 2",
}
occurred
In [It] at: /Users/mradchuk/workspace/src/github.com/operator-framework/operator-controller/test/operator-framework-e2e/operator_framework_test.go:318 @ 08/09/23 10:13:38.056
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@m1kola
The issue is with using podman
.
The error in pushing was due to docker
being hard coded as the container runtime in the test. This is now changed to get the current container runtime using the environment variable CONTAINER_RUNTIME
. The default value of CONTAINER_RUNTIME
defined in the Makefile
is docker
. Therefore the correct runtime has to be assigned to the variable CONTAINER_RUNTIME
before calling the make target if docker
is what is not being used. The test routine also assumes the runtime as docker
if it is unable to retrieve the value of the environment variable.
But this is only a partial fix to the problem. With this change the test for plain+v0
bundles will pass but for registry+v1
will fail. This is because registry+v1
uses operator-sdk
support. Thus to mimic the user experience, the targets bundle-build
and bundle-push
from the generated Makefile by operator-sdk
tool, which has docker
being hard coded as the container runtime, is used to build and push the bundle images. This could be marked as an issue and addressed when hard coding docker
as container runtime in the generated Makefile
is addressed by operator-sdk
.
In conclusion, the current test will only work for docker
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a symlink from docker
to podman
and Podman's CLI is compatible with docker. So make operator-developer-e2e CONTAINER_RUNTIME=podman
and make operator-developer-e2e
give me the same results.
It has to be something else.
I don't know how often (if at all) we will be running this test locally. It might be not worth spinning wheels on this (at least in context of this PR).
bba9a01
to
f3fce19
Compare
6785df4
to
8011376
Compare
I tried running this on my Ventura Mac (M1 Pro):
I tried running this on my F39 (x86_64):
|
Makefile
Outdated
.PHONY: stop-local-registry | ||
stop-local-registry: ## Stop local registry | ||
$(CONTAINER_RUNTIME) container stop local-registry | ||
|
||
.PHONY: remove-local-registry | ||
remove-local-registry: ## Remove local registry | ||
$(CONTAINER_RUNTIME) container rm -v local-registry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having these as separate targets means two things:
They aren't cleaned up when an error occurs, which is good if you want to look at the status of an error, but bad if you want to run the tests again. You might want to force stop in the deploy-local-registry. Or possibly combine these into a single "cleanup" target to make it easier to clean up after failure,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added cleanup-local-registry
which combines both the targets mentioned above. Need to look into force stop in deploy-local-registry if that is required to be addressed.
The podman issue is due to reasons specified in the comment. The test case failure is now fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI if failing at the moment:
Status conditions for the operator registry-operator for the version 0.1.1 is not as expected:Expected status: True, but got: Unknown
Once this failure is deailt with - I think we will be in a good place to get it merged. After that we will be able to iterate in smaller PRs.
I wasn't able to run this locally on ARM mac with podman, but I'm not sure whether we want to hold this PR because of this.
Signed-off-by: jubittajohn <jujohn@redhat.com>
Signed-off-by: jubittajohn <jujohn@redhat.com>
…ADME Signed-off-by: jubittajohn <jujohn@redhat.com>
Signed-off-by: jubittajohn <jujohn@redhat.com> Refractored the plain bundle and registry bundle interactions; Updated the comments and README Signed-off-by: jubittajohn <jujohn@redhat.com> Added make target to install operator-sdk; Added make target to deploy a local registry server; Changed test to use the local registry server Signed-off-by: jubittajohn <jujohn@redhat.com>
…s to be consistent in styling Signed-off-by: jubittajohn <jujohn@redhat.com>
0d30cd3
to
321525f
Compare
…e;Restructured code to have a single It block in a describe block; Signed-off-by: jubittajohn <jujohn@redhat.com> Combined local-registry cleanup make target; Added support for container runtime in consideration Signed-off-by: jubittajohn <jujohn@redhat.com> Bumped the upgrade version from 0.1.1 to 0.2.0 Signed-off-by: jubittajohn <jujohn@redhat.com> Updated comments and README Signed-off-by: jubittajohn <jujohn@redhat.com>
321525f
to
88776d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I think we are in the state where we can merge and we will be able to iterate on top of what you built (e.g. fixing issues with local podman on mac os, etc).
Thank you for all your contributions, best of luck with your studies and I hope to see you around again at some point :)
/lgtm |
Description
This PR is for a cross-component demo with all OLM v1 repositories. The ginkgo test does the following:
plain+v0
bundles and create catalogs to include the bundles.plain+v0
operator.registry+v1
bundles and create catalogs to include the bundles.registry+v1
operator.Purpose of the E2E tests.
Workflow
Checks to be ensured:
Added new Makefile target/Github action:
operator-developer-e2e
test-e2e
are only for testing the functionality of operator-controller. The new e2e test is meant for an operator-framework e2e or an OLM v1 e2e and to capture the developer experience.How to run
operator-developer-e2e
: Runs the entire E2E setup.test-op-dev-e2e
: Runs the ginkgo test.deploy-local-registry
: Deploys local registrycleanup-local-registry
: Stops and removes local registrykind-cluster-cleanup
: Deletes the kind clusterGetting Started
Below are the input values used in the test.
Building operator-controller, deploying it into the cluster and rest of the configuration is done in the
MakeFile
of this repo under the targetoperator-developer-e2e
. This includes:operator-controller-op-dev-e2e
.opm
,operator-sdk
andkustomize
using bingo.The following structs defined are required as input for both plain+v0 and registry+v1 bundles:
baseFolderPath
- Base/root path of the folder for the specific bundle type input data.[path to plain-v0 or registry-v1 bundles testdata]bundles
- Stores the data relevant to different versions of the bundle.bInputDir
- The input directory containing the specific version of the bundle data.bundleVersion
- The specific version of the bundle data.imageRef
- This is formed. Stores the bundle image reference which will be of the formatlocalhost:5001/<operator_name>-bundle:v.<bundleVersion>
baseFolderPath
- Base/root path of the folder for the catalogs.operatorName
- Name of the operator to be installed from the bundles.desiredChannelName
- Desired channel name for the operator.catalogDir
- This is formed. The directory to store the FBC. The formed value will be of the format:<operator-name>-catalog
imageRef
- This is formed. Stores the FBC image reference which will be of the format:localhost:5001/<username>/<catalogDir>:test
fbcFileName
- Name of the FBC file. This is hard-coded ascatalog.yaml
.For getting information related to the install/upgrade action for operators:
type OperatorActionInfo struct { installVersion string upgradeVersion string }
-
installVersion
- Version of the operator to be installed on the cluster.-
upgradeVersion
- Version of the operator to be upgraded on the cluster.Reviewer Checklist