Skip to content
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.

Sign up for GitHub

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

Jump to bottom

e2e: topologyupdater: fix and stabilize tests #961

Merged
merged 3 commits into from
Nov 21, 2022
Merged

e2e: topologyupdater: fix and stabilize tests #961

merged 3 commits into from
Nov 21, 2022

Conversation

Tal-or
Copy link
Contributor

@Tal-or Tal-or commented Nov 17, 2022

A couple of commits which are addressing several issues in the tests.
We explained the rationale for every change in the commits themselves

Signed-off-by: Talor Itzhak titzhak@redhat.com

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 17, 2022
@netlify
Copy link

netlify bot commented Nov 17, 2022 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name Link
🔨 Latest commit 546d0c3
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/637b526a8b3e73000b7e1fd0
😎 Deploy Preview https://deploy-preview-961--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 17, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @Tal-or. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 17, 2022
@Tal-or Tal-or mentioned this pull request Nov 17, 2022
Merged
@ffromani
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 18, 2022
@ffromani ffromani mentioned this pull request Nov 18, 2022
Closed
marquiz
marquiz reviewed Nov 18, 2022
View reviewed changes
Copy link
Contributor

@marquiz marquiz left a comment

Choose a reason for hiding this comment

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

Just one note, otherwise looks good to go

test/e2e/node_feature_discovery.go Outdated
@@ -100,6 +101,7 @@ func cleanupNode(cs clientset.Interface) {
// Actual test suite
var _ = SIGDescribe("Node Feature Discovery", func() {
f := framework.NewDefaultFramework("node-feature-discovery")
f.NamespacePodSecurityEnforceLevel = admissionapi.LevelPrivileged
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, for nfd-master and nfd-worker we shouldn't need this, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we have a choice, but we can't continue with the existing configuration since the test is falling right at the beginning. The choices are:

  1. Adding to the pod spec all the necessary security constraints such as:
    RunAsNonRoot: true, AllowPrivilegeEscalation: false, etc.
  2. Edit the namespace as we have here. It's the simplest solution and since this is only for testing purposes I assumed it's good enough.

Anyhow, we can go with option number 1 as well. Please let me know what is your preference.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Adding to the pod spec all the necessary security constraints such as:
    RunAsNonRoot: true, AllowPrivilegeEscalation: false, etc.

Ach, these are missing in the e2e pod spec 🤦 I think we definitely want to add them and this would be the preferable solution. This way the e2e will also more closely resemble the actual (default) deployment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Please consider that some tests are still falling but are in the node_feature_discovery.go division (which I am unfortunately not too experienced with).
Issue: #962

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But IMVHO I don't think it should stop us from moving on with this PR

@Tal-or
e2e: podSecurity: adapt PodSecurity constraints
a65278d 
The tested pods have some lax spec wrt security,
hence a restrict podSecurity namespace won't allow running those pods.

In topology-updater tests, the topology-updater pod
needs to run the container as root
so change the namespace podSecurity from restricted to priviliged.

In node-feature-discovery tests, we don't need root access,
so add the required security context configuration.

Signed-off-by: Talor Itzhak <titzhak@redhat.com>
@Tal-or
e2e: topologyupdater: insert condition check into eventually
1b6fad6 
We might not get the most updated node topology
resource on the first `GET` call.
Hence, put the whole check inside `Eventually`,
and check for the most updated node topology resource on every
iteration.

Signed-off-by: Talor Itzhak <titzhak@redhat.com>
@Tal-or
e2e: topologyupdater: wait for deletion
546d0c3 
It might take time for the CRD to get deleted
and it might cause some falkiness in the tests.

Now before we create the CRD, we make sure to delete
the old object, wait for it deletion to complete
and only then create a new CRD object.

Signed-off-by: Talor Itzhak <titzhak@redhat.com>
marquiz
marquiz approved these changes Nov 21, 2022
View reviewed changes
Copy link
Contributor

@marquiz marquiz left a comment

Choose a reason for hiding this comment

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

Thank you @Tal-or for improving the e2e tests!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marquiz, Tal-or

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 21, 2022
@marquiz
Copy link
Contributor

marquiz commented Nov 21, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 21, 2022
@k8s-ci-robot k8s-ci-robot merged commit 2c0b6f3 into kubernetes-sigs:master Nov 21, 2022
@Tal-or Tal-or deleted the fix_topologyupdater_e2e branch November 21, 2022 11:56
@marquiz marquiz mentioned this pull request Dec 20, 2022
Closed
22 tasks
@Tal-or Tal-or mentioned this pull request Mar 6, 2023
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marquiz marquiz marquiz approved these changes

@jjacobelli jjacobelli Awaiting requested review from jjacobelli

@fmuyassarov fmuyassarov Awaiting requested review from fmuyassarov

Assignees

@marquiz marquiz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Tal-or @k8s-ci-robot @ffromani @marquiz