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

CNF-16537:Adjust serial suite for running on HyperShift platform [tier0] #1155

Merged
merged 7 commits into from
Jan 29, 2025
Merged

CNF-16537:Adjust serial suite for running on HyperShift platform [tier0] #1155

merged 7 commits into from
Jan 29, 2025

Conversation

Tal-or
Copy link
Collaborator

@Tal-or Tal-or commented Jan 16, 2025 •
edited
Loading

This PR contains the basic changes that are needed for adjusting the serial e2e suite for running on HyperShift platform.

The general idea is to reduce the usage in MachineConfigPool objects and move toward the PoolName API which is a common concept for both HyperShift and OpenShift.

NOTE: The changes here were tested only against tier0 tests.

More details are captured in each commit.

Signed-off-by: Talor Itzhak [email protected]

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 16, 2025
@openshift-ci openshift-ci bot requested review from ffromani and shajmakh January 16, 2025 14:54
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 16, 2025
@Tal-or Tal-or force-pushed the serial_suite_hcp branch 2 times, most recently from 1431182 to b476781 Compare January 23, 2025 13:20
@Tal-or Tal-or changed the title WIP: Adjust serial suite for running on HyperShift platform Adjust serial suite for running on HyperShift platform Jan 23, 2025
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 23, 2025
@Tal-or Tal-or changed the title Adjust serial suite for running on HyperShift platform CNF-16537:Adjust serial suite for running on HyperShift platform Jan 27, 2025
@openshift-ci-robot
Copy link

@Tal-or: This pull request references CNF-16537 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.19.0" version, but no target version was set.

In response to this:

This PR contains the basic changes that are needed for adjusting the serial e2e suite for running on HyperShift platform.

The general idea is to reduce the usage in MachineConfigPool objects and move toward the PoolName API which is a common concept for both HyperShift and OpenShift.

More details are captured in each commit.

Signed-off-by: Talor Itzhak [email protected]

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.

@Tal-or Tal-or force-pushed the serial_suite_hcp branch 3 times, most recently from 1acc8cd to f1b36e2 Compare January 27, 2025 14:46
@openshift-ci-robot
Copy link

@Tal-or: This pull request references CNF-16537 which is a valid jira issue.

In response to this:

This PR contains the basic changes that are needed for adjusting the serial e2e suite for running on HyperShift platform.

The general idea is to reduce the usage in MachineConfigPool objects and move toward the PoolName API which is a common concept for both HyperShift and OpenShift.

NOTE: The changes here were tested only against tier0 tests.

More details are captured in each commit.

Signed-off-by: Talor Itzhak [email protected]

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.

@openshift-ci-robot
Copy link

@Tal-or: This pull request references CNF-16537 which is a valid jira issue.

In response to this:

This PR contains the basic changes that are needed for adjusting the serial e2e suite for running on HyperShift platform.

The general idea is to reduce the usage in MachineConfigPool objects and move toward the PoolName API which is a common concept for both HyperShift and OpenShift.

NOTE: The changes here were tested only against tier0 tests.

More details are captured in each commit.

Signed-off-by: Talor Itzhak [email protected]

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.

@Tal-or
client: add log in case of error
a91f919 
make debugging easier and explain the reason for the
failure.

Signed-off-by: Talor Itzhak <[email protected]>
@Tal-or Tal-or mentioned this pull request Jan 27, 2025
Merged
@Tal-or Tal-or changed the title CNF-16537:Adjust serial suite for running on HyperShift platform CNF-16537:Adjust serial suite for running on HyperShift platform [tier0] Jan 27, 2025
ffromani
ffromani reviewed Jan 27, 2025
View reviewed changes
Copy link
Member

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

most of the changes LGTM but we need to review how and where we move code around. Didn't review the last commit yet

Tal-or added 2 commits January 27, 2025 17:39
@Tal-or
kubeletconfig: move decode func to kubeletconfig package
597d9df 
Later on we'll need this helper function elsewhere,
so let's make it public and move
it into more correct place.

Signed-off-by: Talor Itzhak <[email protected]>
@Tal-or
internal:hypershift: add consts
9a89835 
add a package of hypershift consts which lists useful
constants that can be used across the payload and test code.

each const that is added to the package shall be documented
so its purpose and usage will be clear.

Signed-off-by: Talor Itzhak <[email protected]>
@ffromani
Copy link
Member

the general rule is:

  1. test/utils/...: code which used ONLY by tests. Try hard to resist to add code here (utils is an anti-pattern: https://dave.cheney.net/2019/01/08/avoid-package-names-like-base-util-or-common) use internal or pkg. Code in test/utils/... can depend on any package
  2. internal/...: code which is used ONLY by other code in the repo. Code in internal/... can depend on any package
  3. pkg/...: code which we are confident or we plain recommend to use from external client. We provide soft-stability guarantee: we try our hardest to NOT break backward compat, but we can't really promise and we rarely break if we have strong reasons to (project: kubebuilder layout v3 -> v4 #1160 ). Code in pkg/... can depend only on other public-facing code, like 3rd party imports or api/....

@Tal-or
Copy link
Collaborator Author

Tal-or commented Jan 27, 2025 •
edited
Loading

the general rule is:

test/utils/...: code which used ONLY by tests. Try hard to resist to add code here (utils is an anti-pattern: https://dave.cheney.net/2019/01/08/avoid-package-names-like-base-util-or-common) use internal or pkg. Code in test/utils/... can depend on any package
internal/...: code which is used ONLY by other code in the repo. Code in internal/... can depend on any package
pkg/...: code which we are confident or we plain recommend to use from external client. We provide soft-stability guarantee: we try our hardest to NOT break backward compat, but we can't really promise and we rarely break if we have strong reasons to (#1160 ). Code in pkg/... can depend only on other public-facing code, like 3rd party imports or api/....

Thanks, and what about code which is not included in any of the above?
like nrovalidate/validator/kubeletconfig.go for example

@ffromani
Copy link
Member

ffromani commented Jan 27, 2025 •
edited by Tal-or
Loading

the general rule is:

test/utils/...: code which used ONLY by tests. Try hard to resist to add code here (utils is an anti-pattern: https://dave.cheney.net/2019/01/08/avoid-package-names-like-base-util-or-common) use internal or pkg. Code in test/utils/... can depend on any package
internal/...: code which is used ONLY by other code in the repo. Code in internal/... can depend on any package
pkg/...: code which we are confident or we plain recommend to use from external client. We provide soft-stability guarantee: we try our hardest to NOT break backward compat, but we can't really promise and we rarely break if we have strong reasons to (#1160 ). Code in pkg/... can depend only on other public-facing code, like 3rd party imports or api/....

Thanks, and what about code which is not included in any of the above? like nrovalidate/validator/kubeletconfig.go for example

that's top-level code, so it can use internal/... and pkg/... but not test/utils/.... Nothing outside tests/... can import test/utils/...

@Tal-or
Copy link
Collaborator Author

Tal-or commented Jan 27, 2025

@ffromani Done.

I must admit it's hard keep tracking the all the above rule, maybe we should add some CI lane to force it.
Anyway this is an enhancement proposal for another PR and another day

Tal-or added 4 commits January 27, 2025 18:52
@Tal-or
nodegroups:internal: add helper package
3ff8823 
add internal helper package to interact easily with nodegropus,
and exctract different k8s resources associate with it.

Signed-off-by: Talor Itzhak <[email protected]>
@Tal-or
validator: validate kubelet data for hypershift
d986e3a 
expend the CollectKubeletConfig function to support
both OpenShift and HyperShift platform.

Signed-off-by: Talor Itzhak <[email protected]>
@Tal-or
infra: use poolnames
0f3b426 
The setup code is heavily integrated with MCPs.
This is good enough for OpenShift but a no-go
for HyperShift.

Hence, lets generalize the flow to use PoolName
instead, which is acceptable for both platforms.

Signed-off-by: Talor Itzhak <[email protected]>
@Tal-or
serial:baseload: add baseload accounting to padding pods
3417b56 
In some of the tests we were skipping the basload accounting
this in turn led for some of the padding pod to keep
pending.

This commit fixes the problem by subtracting the baseload resources
from the padding pod request.

Signed-off-by: Talor Itzhak <[email protected]>
ffromani
ffromani reviewed Jan 29, 2025
View reviewed changes
Copy link
Member

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

thanks!

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2025
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 29, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ffromani, 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

@openshift-merge-bot openshift-merge-bot bot merged commit b298ca5 into openshift-kni:main Jan 29, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ffromani ffromani ffromani left review comments

@shajmakh shajmakh Awaiting requested review from shajmakh

Assignees

@ffromani ffromani

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Tal-or @openshift-ci-robot @ffromani