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.

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

Fix test issues + increase number of nodes in the cluster #241

Merged
merged 6 commits into from
Apr 30, 2024

Conversation

ShellyKa13
Copy link
Contributor

@ShellyKa13 ShellyKa13 commented Apr 17, 2024

What this PR does / why we need it:
We have seen several test issues when running with more then 1 nodes.
This PR fixes those issues and increase the number of nodes running in the CI to better catch this issues.
Changes:

  • Fixed pod security and arranged pod related functionallities (should solve failures in test 9864)
  • Fixed issue in test 10191. jira-ticket: https://issues.redhat.com/browse/CNV-39913
  • Remove debug code that causes timeouts d/s when the test fails there.
  • Update instancetype and preferences manifests version since alpha is deprecated and will be removed soon

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes # test 9864 and 10191 d/s

Special notes for your reviewer:

Release note:

Create cluster with 2 nodes

@kubevirt-bot kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Apr 17, 2024
@kubevirt-bot kubevirt-bot requested review from awels and mhenriks April 17, 2024 12:02
@ShellyKa13 ShellyKa13 removed the request for review from awels April 17, 2024 12:02
@mhenriks
Copy link
Member

/lgtm
/approve

@kubevirt-bot kubevirt-bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 18, 2024
@kubevirt-bot kubevirt-bot added size/L and removed lgtm Indicates that a PR is ready to be merged. size/S labels Apr 21, 2024
@ShellyKa13
Copy link
Contributor Author

/hold
Changed number of nodes in the cluster, I want to run several times to see its stable

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 21, 2024
@ShellyKa13 ShellyKa13 changed the title Fix test pod security + Update version of instancetype and prefernce yamls Fix test issues + increase number of nodes in the cluster Apr 21, 2024
Copy link
Member

@awels awels left a comment

Choose a reason for hiding this comment

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

I know you mostly just moved some stuff around, but since you are improving the code maybe look at some of the nits. I am fine if you ignore me.

},
Spec: v1.PodSpec{
SecurityContext: &v1.PodSecurityContext{
FSGroup: pointer.Int64(uid),
Copy link
Member

Choose a reason for hiding this comment

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

I think the pointer library is deprecated and you should use the ptr library k8s.io/utils/ptr. This becomes something like this

ptr.To[int64](uid)

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

},
},
SecurityContext: &v1.SecurityContext{
RunAsNonRoot: pointer.Bool(true),
Copy link
Member

Choose a reason for hiding this comment

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

same with pointer to ptr

RunAsNonRoot : ptr.To[bool](true)

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

tests/framework/pod.go Show resolved Hide resolved
@kubevirt-bot kubevirt-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL and removed size/L labels Apr 30, 2024
First write to the pvc and only after start the vm,
this will prevent the option of pvc already mounted to
the virt-launcher and then the writer-pod might fail
writing from another pod it will be scheduled on.
Also delete redundant stop of vm before deletion.
Fixes: https://issues.redhat.com/browse/CNV-39913

Signed-off-by: Shelly Kagan <[email protected]>
*Remove duplicated function (NewPod)
*Fix pod security issues by using function that already handles that
*Move pod functions to new file
*Create and use wait for pod phase function

Signed-off-by: Shelly Kagan <[email protected]>
Signed-off-by: Shelly Kagan <[email protected]>
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2024
Copy link
Member

@awels awels left a comment

Choose a reason for hiding this comment

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

/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2024
@kubevirt-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awels, mhenriks

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

@ShellyKa13
Copy link
Contributor Author

/unhold

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 30, 2024
@kubevirt-bot kubevirt-bot merged commit 2c6758b into kubevirt:main Apr 30, 2024
5 checks passed
@ShellyKa13
Copy link
Contributor Author

/cherrypick release-v0.6

@kubevirt-bot
Copy link

@ShellyKa13: #241 failed to apply on top of branch "release-v0.6":

Applying: Update version of instancetype and prefernce yamls to use beta instead of alpha
Applying: Move start of vm to after writer pod succeeded
Applying: Gather all pod related functions in one place
Using index info to reconstruct a base tree...
M	tests/framework/storage.go
M	tests/resource_filtering_test.go
Falling back to patching base and 3-way merge...
Auto-merging tests/resource_filtering_test.go
Auto-merging tests/framework/storage.go
CONFLICT (content): Merge conflict in tests/framework/storage.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 Gather all pod related functions in one place
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-v0.6

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants