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

CNF-11193: controller:hypershift: add support in for Kubelet controller #1015

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Tal-or
Copy link
Collaborator

@Tal-or Tal-or commented Sep 17, 2024

Kubelet controller is responsible for extracting the TM configuration from the
KubeletConfig object, craft a ConfigMap from it so it could be consumed later on
by the RTE workload.

On HyperShift there's no plain KubeletConfig object but a ConfigMap that encapsulate it instead,

This PR contains all the changes that were needed for the controller to be able to provide
similar functionality when it's running on HyperShift platform.

Copy link
Contributor

openshift-ci bot commented Sep 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 17, 2024
@Tal-or
Copy link
Collaborator Author

Tal-or commented Sep 17, 2024

/hold
depends on openshift/enhancements#1671

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 17, 2024
@Tal-or
Copy link
Collaborator Author

Tal-or commented Sep 17, 2024

unit test probably failed as well, I'm facing some issues running them locally

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.

overall looks OK. Need to deep dive into the tests mostly

@Tal-or
Copy link
Collaborator Author

Tal-or commented Sep 17, 2024

overall looks OK. Need to deep dive into the tests mostly

Take your time.
This PR has a long wait in the queue anyway

Latest version allow detecting HyperShift platform.

Signed-off-by: Talor Itzhak <[email protected]>
Add the platform field to the reconciler so it would
be able to detect whether it run on OCP or HCP.

Signed-off-by: Talor Itzhak <[email protected]>
Add the functionality to watch for ConfigMap changes when running on HCP.
On HCP the controller should not watch for the bare KubeletConfig
object, but for the ConfigMap that hold it.

Signed-off-by: Talor Itzhak <[email protected]>
The business logic of kubelet controller is similar when running on OCP or HCP.
However there are slighty different parameters that some of the
existing functions in the reconcile loop accepts.

We introduce the `kubeletConfigHandler` so we could maintain a common
API for the functions for both OCP and HCP platforms.

Signed-off-by: Talor Itzhak <[email protected]>
The `makeKCHandlerForPlatform` provides a common handler that can
be used on both HCP and OCP platform.

This way allow us to narrow down the changes in the logic in to
a centeral close-scoped function in the code,
without having the need to branch the logic for HCP and OCP
in other areas in the Reconcile loop.

Signed-off-by: Talor Itzhak <[email protected]>
Change the signature to accept kubeletConfigHandler object.

Signed-off-by: Talor Itzhak <[email protected]>
Put the kubeletconfig controller tests inside a DescribeTable
where one entry is for OCP and another for HCP.

Signed-off-by: Talor Itzhak <[email protected]>
@Tal-or Tal-or changed the title controller:hypershift: add support in for Kubelet controller CNF-11193: controller:hypershift: add support in for Kubelet controller Sep 17, 2024
@openshift-ci-robot
Copy link

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

In response to this:

Kubelet controller is responsible for extracting the TM configuration from the
KubeletConfig object, craft a ConfigMap from it so it could be consumed later on
by the RTE workload.

On HyperShift there's no plain KubeletConfig object but a ConfigMap that encapsulate it instead,

This PR contains all the changes that were needed for the controller to be able to provide
similar functionality when it's running on HyperShift platform.

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
Copy link
Collaborator Author

Tal-or commented Sep 17, 2024

/retest

1 similar comment
@Tal-or
Copy link
Collaborator Author

Tal-or commented Sep 18, 2024

/retest

@ffromani
Copy link
Member

/test ci-e2e

Copy link
Contributor

openshift-ci bot commented Sep 19, 2024

@Tal-or: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/ci-install-e2e c9d4690 link true /test ci-install-e2e
ci/prow/ci-e2e c9d4690 link true /test ci-e2e

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.

@openshift-merge-robot
Copy link
Contributor

PR needs rebase.

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.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2024
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. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants