-
Notifications
You must be signed in to change notification settings - Fork 105
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
CNF-11817: e2e: control and data planes clients #1004
CNF-11817: e2e: control and data planes clients #1004
Conversation
@Tal-or: This pull request references CNF-11817 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. In 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. |
/test e2e-gcp-pao |
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.
initial review, so far so good
d566809
to
0dafc42
Compare
236f141
to
99f2f66
Compare
be6cd25
to
77a146b
Compare
/retest |
1 similar comment
/retest |
77a146b
to
f238a4d
Compare
f238a4d
to
0ec07de
Compare
On hypershift, there is a full separation between the control and data plane. This means there's a separate client for each. This commit implements the control plane client. In order to preserve compatibility between HCP and OCP, the client encodes/decodes the objects in/from a `ConfigMap` automatically. This allows the user to perform similar CRUD operations without having the need to distinguish between HCP and OCP platforms. Signed-off-by: Talor Itzhak <[email protected]>
On Hypershift we should be able to access both the management and hosted clusters. For that, we should now have two clients, each communicates with a different cluster. ControlPlaneClient - defines the API client to run CRUD operations on the control plane that will be used for testing. DataPlaneClient - defines the API client to run CRUD operations on the data plane that will be used for testing. On non-hypershift both client would point to the same the cluster. Also added Ginkgo assertions that prevent the user from using the `DataPlaneClient` client for the wrong object. For example using the `DataPlaneClient` for querying the `PerformanceProfile` object is completley fine on non-hypershift cluster, but will fail badly on hypershift (since `PerformanceProfile` objects, are not presented on the hosted clusters. Signed-off-by: Talor Itzhak <[email protected]>
Added functions for building the clients for hypershift while running on CI environment Signed-off-by: Talor Itzhak <[email protected]>
@Tal-or: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
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 initial unused implementation looks good to me.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MarSik, 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 |
/label acknowledge-critical-fixes-only This is required infrastructure preparation to be included in 4.16 . currently not wired to existing functionality |
[ART PR BUILD NOTIFIER] This PR has been included in build cluster-node-tuning-operator-container-v4.17.0-202405161113.p0.gf37f16f.assembly.stream.el9 for distgit cluster-node-tuning-operator. |
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.
overall LGTM as well
return err | ||
} | ||
var objAsYAML string | ||
// can't have both |
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.
but we don't check this, and also we don't check we have either.
Well, the decoder does implicitely I guess
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.
There's a room for improvement indeed. I'll add it in a follow PR. Anyhow this code is not wired yet
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, no big deal
On Hypershift we should be able to access both the management and hosted clusters.
For that, we should now have two clients, each communicates with a different cluster.
ControlPlaneClient - defines the API client to run CRUD operations on the control plane that will be used for testing.
DataPlaneClient - defines the API client to run CRUD operations on the data plane that will be used for testing.
On non-hypershift both client would point the same the cluster under the hood.
We also added Ginkgo assertions that prevent the user from using the wrong clients for the wrong objects.
For example using the
DataPlaneClient
for querying thePerformanceProfile
object is completley fine on non-hypershift cluster, but will fail badly on hypershift (sincePerformanceProfile
objects, are not presented on the hosted clusters.