-
Notifications
You must be signed in to change notification settings - Fork 58
Create OLM upgrade e2e scenario using codeflare SDK #286
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
Create OLM upgrade e2e scenario using codeflare SDK #286
Conversation
a02add7
to
851164e
Compare
a122ebf
to
182a440
Compare
@sutaakar OLM tests are Failing due to lack of resources in KinD cluster and test are pass in local.. I think we can enable this tests only when Large runners are available |
@Srihari1192 Last part of the log:
Can you confirm whether SDK supports using Ingress? If so, can you check that Ingress is properly created when using KinD with Ingress installed in the test setup? |
Sure |
@sutaakar SDK not supporting Ingress yet.. Implementation is in progress for this project-codeflare/codeflare-sdk#251 |
182a440
to
a1432a2
Compare
test/e2e/olm_upgrade_test.go
Outdated
|
||
test := With(t) | ||
test.T().Parallel() | ||
if os.Getenv("RUN_OLM_TESTS") != "true" { |
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.
It may be better to use build tags - https://stackoverflow.com/questions/54165975/go-test-only-run-tests-that-contain-a-build-tag
That way you can specify the OLM upgrade tests when invoking the tests, i.e. go test -tags olm_upgrade_test ...
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 disadvantage of this approach is that the file won't compile if the tag is not enabled.....
Thinking whether it may be better just to remove the condition and specify what tests to run in makefile - to have a separate command there to run upgrade tests.
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.
Yes also if we are adding tags, need to adjust to all the e2e tests with build tag to skip these tests running as part of e2e
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.
@sutaakar Probably we can use Test grouping for e2e test run like go test -timeout 30m -v ./test/e2e -run "^TestMNIST.*$"
as all our e2e tests starts with TestMNIST and rename OLM upgrade test to TestOLMUpgradeRayClusterUp and TestOLMUpgradeMnistJobSubmit . So that we can call these tests specifically in our workflows by removing the condition
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.
Or maybe this test can be moved to dedicated folder, i.e. test/upgrade
.
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.
Sure will go with this approach
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.
Moved the tests to folder test/upgrade
.. kept test dependent files in the test/e2e
as ReadFile method excepts files to be in the same package
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.
Thinking whether it would have sense to copy the method (doesn't have to be exported) to the upgrade
package.
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.
okay left as same as existing
dfb97dd
to
a1432a2
Compare
4ee49db
to
b54e5b5
Compare
d94ed8f
to
1a80a6c
Compare
1a80a6c
to
1bda26e
Compare
1bda26e
to
5cdcb30
Compare
e7d10ed
to
13e88a9
Compare
13e88a9
to
f967729
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.
/lgtm
@astefanutti do you have any feedback for this PR, or should we merge it? |
defer func() { | ||
if t.Failed() { | ||
DeleteTestNamespace(test, namespace) | ||
} else { | ||
StoreNamespaceLogs(test, namespace) | ||
} | ||
}() |
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.
@Srihari1192 @sutaakar out of curiosity, why not using the "standard" way, where test support does that automatically?
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.
@astefanutti In this Upgrade context, we are using the same namespace in after operator upgrade test. As NewTestNamespace will delete the namespace by default after test complete , so we added this supported methods in codeflare-common
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.
@Srihari1192 Thanks, that's clear now.
Two things I could suggest we could lean on in the future:
- Rely on the options argument of the
NewTestNamespace
method, to provide the name or prevent deletion for example, instead of creating ad-hoc methods. Options enable to mix things. - The test logic seems fragmented between the GH Actions workflow, and the Go tests. It may be better to implement the upgrade as part of the Go test, so it's not necessary to deal with namespace deletion and run tests by name.
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.
It may be better to implement the upgrade as part of the Go test
This will couple the test with specific upgrade strategy (using OLM, overriding existing deployment with new oneliner, upgrade using ODH). Personally I would prefer keep test implementation aside from deployment/upgrade, to keep the test reusable for any strategy.
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.
It makes sense. Sounds good 👍🏼.
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: astefanutti The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Issue link
#184
What changes have been made
Verification steps
Checks