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

Enhance Cluster Environment Detection for OSTree-Based and Classic RHEL Systems #215

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

Conversation

SamD2021
Copy link

@SamD2021 SamD2021 commented Nov 21, 2024

This PR shifts the DPU Operator's focus from distinguishing between OpenShift and MicroShift flavors to a more robust approach of identifying the system type (Classic RHEL vs. OSTree-based RHEL). It improves the operator's ability to interact with immutable file systems, ensuring compatibility with modern RHEL variations, including RHEL Image Mode and RHEL CoreOS on DPUs.

cluster_environment.go:

Transitioned flavor logic from "OpenShift vs. MicroShift" to identifying "Classic RHEL vs. OSTree-based RHEL."
This change enables the operator to dynamically assess whether the filesystem is immutable and adjust its operations accordingly.
Added support for deployments on RHEL Image Mode and other OSTree-based setups running on the DPU.

path_manager.go:

Updated internal/utils/path_manager.go to align paths with their respective system flavors.
Adjusted the Classic RHEL flavor to use paths previously associated with the MicroShift flavor.
Allowed OSTree-based systems to leverage the /opt path restrictions and default to OpenShift-like paths.

99.daemonset.yaml:

Modified 99.daemonset.yaml to introduce a new host-run mount path for /var/run, by also renaming the mount path /var/run/netns to host-run-netns
Added run/ostree-booted to confirm the presence of OSTree environments without creating unnecessary files that might result in false positives.

This PR has been tested on an Intel IPU with 1.8.0 MeV running RHEL Image Mode, where it successfully detects the Ostree flavor and retrieves the correct CNI path.

Copy link

openshift-ci bot commented Nov 21, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: SamD2021
Once this PR has been reviewed and has the lgtm label, please assign bn222 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 21, 2024
Copy link

openshift-ci bot commented Nov 21, 2024

Hi @SamD2021. Thanks for your PR.

I'm waiting for a openshift 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-sigs/prow repository.

@bn222
Copy link
Contributor

bn222 commented Nov 21, 2024

Microshift can be installed on RHEL for edge environments, which operates similarly to an immutable system. However, the suggested method may not be able to differentiate between various OpenShift deployments.

@SamD2021
Copy link
Author

Microshift can be installed on RHEL for edge environments, which operates similarly to an immutable system. However, the suggested method may not be able to differentiate between various OpenShift deployments.

Would it be best then to keep all detection of microshift, openshift, classic RHEL and Ostree-based deployments, and keep it along side these changes, so when we need something like in the case of running Microshift on RHEL For Edge, we can just add to the case to match Ostree flavours to be able to assign the Immutable-friendly path for the CNI and still allow for differentiating the Microshift flavour for specific Microshift related functionality?

@SalDaniele
Copy link
Contributor

/ok-to-test

@openshift-ci openshift-ci bot 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 21, 2024
@SalDaniele
Copy link
Contributor

Do we need to differentiate between different Openshift deployments?

We only detect flavour currently to determine the proper CNI path. I think this requires knowing "are we on an immutable file system or not" not the current way this is implemented "are we on openshift or microshift".

But maybe I am not considering something here.

@openshift-ci-robot
Copy link

/test remaining-required

…from microshift vs openshift

The reason why we had the flavours as Openshift vs Microshift is that we
assumed that if the DPU Operator is running on Openshift then it runs an
immutable operating system, which in this case is CoreOS. Like wise if
the operator is running on Microshift we assumed that the node is
running classic RHEL (mutable). The assumptions are not wrong in our
current use cases but microshift's target audience seems to be for
immutable flavors of RHEL, so we should make this more robust by
checking whether a system is Classic RHEL or if its an OSTREE-Based
RHEL, since it gives us a better way to know if parts of the filesystem
is writeble or if binaries can even be installed on it by the Operator.
This would also add compatibility with running Microshift on RHEL Image
Mode, RHEL for edge or RHEL CoreOS on the DPU.
…vour

OSTREE-Based immutable systems usually mount `/opt` as read only so we
need it to use the path that the OpenShift flavour was using before.
Likewise we let the ClassicRHEL flavour use the microshift path instead.
- Changed the name of the host-run that mounted the path
`/var/run/netns` to match its functionality
- Added a mountPath for `/var/run` and named it as host-run, so we have
access to the `/run/ostree-booted` and check if it has been created or
not. (Doing it this way lets us avoid any extra file creation that might
give false positives for the `isOstree` function)
… a single flavor

- Retrieving a set of flavours lets us match cases well such as being
able to differentiate if something is microshift + RHEL For Edge or
Openshift + Classic RHEL. (Basically able to match multiple flavours)
@SamD2021 SamD2021 force-pushed the detect-ostree-based branch from eca5cb5 to d66af18 Compare November 22, 2024 20:22
@SamD2021
Copy link
Author

/test all

@openshift-ci-robot
Copy link

/test remaining-required

Copy link

openshift-ci bot commented Nov 22, 2024

@SamD2021: The following test 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/make-e2e-test d66af18 link true /test make-e2e-test

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.


// Add a flavour to the set
func (f FlavourSet) Add(flavour Flavour) {
f[flavour] = struct{}{}
Copy link
Contributor

Choose a reason for hiding this comment

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

struct{}{} needs more thought

// isClassicRHEL checks if the OS running on the current node is classic RHEL.
func (ce *ClusterEnvironment) isClassicRHEL(ctx context.Context) (bool, error) {
// Retrieve the node name from the K8S_NODE environment variable
nodeName := os.Getenv("K8S_NODE")
Copy link
Contributor

Choose a reason for hiding this comment

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

downward API always available?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes downward API is always available in kubernetes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants