SDN-4168: Optimize timing for IPsec tests - #28797
Merged
openshift-merge-bot[bot] merged 3 commits intoDec 21, 2024
Merged
Conversation
pperiyasamy
marked this pull request as draft
May 13, 2024 11:52
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
pperiyasamy
force-pushed
the
fix-ipsec-for-ci
branch
from
May 15, 2024 13:44
9b28169 to
95898fc
Compare
pperiyasamy
changed the title
pperiyasamy
marked this pull request as ready for review
May 15, 2024 13:47
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
pperiyasamy
force-pushed
the
fix-ipsec-for-ci
branch
from
May 15, 2024 13:58
95898fc to
94bd34b
Compare
pperiyasamy
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@pperiyasamy: This pull request references SDN-4168 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Job Failure Risk Analysis for sha: 94bd34b
|
Member
Author
|
/assign @jcaamano |
pperiyasamy
force-pushed
the
fix-ipsec-for-ci
branch
from
May 21, 2024 13:21
94bd34b to
59a8b3a
Compare
|
Job Failure Risk Analysis for sha: 59a8b3a
|
jcaamano
reviewed
May 21, 2024
jcaamano
left a comment
jcaamano
left a comment
Contributor
There was a problem hiding this comment.
@pperiyasamy I think we need to find a way to test this in CI without merging
| func ensureIPsecExternalEnabled(oc *exutil.CLI) error { | ||
| return wait.PollUntilContextTimeout(context.Background(), ipsecRolloutWaitInterval, | ||
| ipsecRolloutWaitDuration, true, func(ctx context.Context) (bool, error) { | ||
| ds, err := getDaemonSet(oc, ovnNamespace, ovnIPsecDsName) |
Contributor
There was a problem hiding this comment.
I would add a comment here that you expect the daemon set to not be deployed
| return nil | ||
| }) | ||
| pingSync.Go(func() error { | ||
| wg.Wait() |
Contributor
There was a problem hiding this comment.
I don't think this wait group is going to help
I would just increase the ping counter if you think we need that
| o.Expect(err).To(o.HaveOccurred()) | ||
| err = pingAndCheckNodeTraffic(config.srcNodeConfig, config.dstNodeConfig, icmp) | ||
| o.Expect(err).To(o.HaveOccurred()) | ||
| err = nil |
Contributor
There was a problem hiding this comment.
I just realized you probably need to consider using GinkgoHelper() if you are going to use gomega on helper functions
https://onsi.github.io/gomega/#making-assertions-in-helper-functions
Member
Author
There was a problem hiding this comment.
The err is expected to happen here. so err = nil is needed to make defer block clean up test pods.
added GinkgoHelper() in helper functions.
| o.Expect(nsCertMachineConfig).NotTo(o.BeNil()) | ||
| o.Eventually(func() bool { | ||
| pools, err := getMachineConfigPoolByLabel(oc, workerRoleMachineConfigLabel) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) |
Contributor
There was a problem hiding this comment.
I just realized your function needs to take a gomega instance as parameter if you plan to use it within Eventually blocks
https://onsi.github.io/gomega/#category-3-making-assertions-eminem-the-function-passed-into-codeeventuallycode
| g.By(fmt.Sprintf("configure IPsec in %s mode and validate traffic", mode)) | ||
| // Change IPsec mode to External and packet capture on the node's interface | ||
| // must be geneve encapsulated ones. | ||
| // Change IPsec mode to given mode and do packet capture on the node's interface |
Contributor
There was a problem hiding this comment.
This err := configureIPsecMode(oc, mode) is not causing a reboot when ipsec machine config extensions are deployed? Is that because we the CI job starts deploys the cluster with IPSec already enabled? In Full mode?
Member
Author
There was a problem hiding this comment.
yes, this is what we discussed last week. We'll have CI job starting cluster with IPsec full mode and then execute the tests with minimal number of node reboots
| if !time.Now().Before(certExpirationDate) { | ||
| framework.Failf("certficates in the Machine Config are expired, Please consider recreating those certificates") | ||
| } | ||
| nsCertMachineConfig, err := createIPsecCertsMachineConfig(oc) |
Contributor
There was a problem hiding this comment.
I guess this does not cause a reboot because they are just ceerts
Member
Author
There was a problem hiding this comment.
this still causes reboot of worker nodes and certs are deployed via machine config.
pperiyasamy
force-pushed
the
fix-ipsec-for-ci
branch
from
May 23, 2024 08:57
59a8b3a to
ff43448
Compare
pperiyasamy
commented
May 23, 2024
Member
Author
There was a problem hiding this comment.
I think we need to find a way to test this in CI without merging
yes, we must merge this CI lane openshift/release#50687 to get this tested. cc @jluhrsen
| func ensureIPsecExternalEnabled(oc *exutil.CLI) error { | ||
| return wait.PollUntilContextTimeout(context.Background(), ipsecRolloutWaitInterval, | ||
| ipsecRolloutWaitDuration, true, func(ctx context.Context) (bool, error) { | ||
| ds, err := getDaemonSet(oc, ovnNamespace, ovnIPsecDsName) |
| o.Expect(err).To(o.HaveOccurred()) | ||
| err = pingAndCheckNodeTraffic(config.srcNodeConfig, config.dstNodeConfig, icmp) | ||
| o.Expect(err).To(o.HaveOccurred()) | ||
| err = nil |
Member
Author
There was a problem hiding this comment.
The err is expected to happen here. so err = nil is needed to make defer block clean up test pods.
added GinkgoHelper() in helper functions.
| if !time.Now().Before(certExpirationDate) { | ||
| framework.Failf("certficates in the Machine Config are expired, Please consider recreating those certificates") | ||
| } | ||
| nsCertMachineConfig, err := createIPsecCertsMachineConfig(oc) |
Member
Author
There was a problem hiding this comment.
this still causes reboot of worker nodes and certs are deployed via machine config.
| g.By(fmt.Sprintf("configure IPsec in %s mode and validate traffic", mode)) | ||
| // Change IPsec mode to External and packet capture on the node's interface | ||
| // must be geneve encapsulated ones. | ||
| // Change IPsec mode to given mode and do packet capture on the node's interface |
Member
Author
There was a problem hiding this comment.
yes, this is what we discussed last week. We'll have CI job starting cluster with IPsec full mode and then execute the tests with minimal number of node reboots
| o.Expect(nsCertMachineConfig).NotTo(o.BeNil()) | ||
| o.Eventually(func() bool { | ||
| pools, err := getMachineConfigPoolByLabel(oc, workerRoleMachineConfigLabel) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) |
Member
Author
|
/test ? |
Contributor
|
@pperiyasamy: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Member
Author
|
/test e2e-aws-ovn-ipsec-serial |
4 similar comments
Member
Author
|
/test e2e-aws-ovn-ipsec-serial |
Member
Author
|
/test e2e-aws-ovn-ipsec-serial |
Member
Author
|
/test e2e-aws-ovn-ipsec-serial |
Member
Author
|
/test e2e-aws-ovn-ipsec-serial |
|
Job Failure Risk Analysis for sha: ff43448
|
|
Job Failure Risk Analysis for sha: 7386625
|
Member
Author
|
/test e2e-aws-ovn-ipsec-serial |
|
Job Failure Risk Analysis for sha: 7386625
|
Contributor
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin, pperiyasamy The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Member
Author
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
Member
Author
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
|
Job Failure Risk Analysis for sha: 7386625
|
|
Job Failure Risk Analysis for sha: 7386625
|
1 similar comment
|
Job Failure Risk Analysis for sha: 7386625
|
Member
Author
|
/retest-required |
1 similar comment
Member
Author
|
/retest-required |
|
Job Failure Risk Analysis for sha: 7386625
|
1 similar comment
|
Job Failure Risk Analysis for sha: 7386625
|
Member
Author
|
/retest-required |
Contributor
|
@pperiyasamy: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Job Failure Risk Analysis for sha: 7386625
|
|
Job Failure Risk Analysis for sha: 7386625
|
Member
Author
|
/retest-required |
Contributor
|
[ART PR BUILD NOTIFIER] Distgit: openshift-enterprise-tests |
Contributor
|
/cherry-pick release-4.18 |
|
@jluhrsen: new pull request created: #29411 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
11 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The ipsec test suite takes about 5-6 hours to finish running the tests, This PR optimizes ipsec tests such that it reboots only worker nodes for cert installation and tests execution time can be greatly reduced to 2-3 hrs.