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

Support for Multiarch Manager Operator #1417

Merged
merged 1 commit into from Aug 21, 2023

Conversation

Prashanth684
Copy link
Contributor

This enhancement proposes a multiarch manager operator which will be a secondary OLM managed operator installable through OperatorHub. The operator will expose functionality that when configured, would allow pods to be scheduled on the appropriate architecture nodes based on the architecture of the container images.

The operator's name is chosen as such in case future functionality would need to be added related to multi-architectural workings.

xref: https://issues.redhat.com/browse/MIXEDARCH-215

Co-authored-by: Alessandro Di Stefano aleskandro@redhat.com

@Prashanth684
Copy link
Contributor Author

/cc @aleskandro

@openshift-ci openshift-ci bot requested a review from aleskandro June 2, 2023 19:44
enhancements/multi-arch/multiarch-manager-operator.md Outdated Show resolved Hide resolved
enhancements/multi-arch/multiarch-manager-operator.md Outdated Show resolved Hide resolved
enhancements/multi-arch/multiarch-manager-operator.md Outdated Show resolved Hide resolved
enhancements/multi-arch/multiarch-manager-operator.md Outdated Show resolved Hide resolved
enhancements/multi-arch/multiarch-manager-operator.md Outdated Show resolved Hide resolved
Comment on lines 563 to 564
the`/etc/containers/policy.json` and `/etc/containers/registries.conf` files in the pod placement controller according
to the settings of the `image.config.openshift.io/cluster` object's registrySources fields
Copy link
Contributor

Choose a reason for hiding this comment

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

(BTW callers of the containers/image library can point it at other locations, so the code does not necessarily need root inside the container to be able to overwrite files in /etc.)

Copy link
Member

Choose a reason for hiding this comment

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

We will not leverage a privileged/rooted pod for changing this folder and will address the actual location of the files at implementation time.

Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

I think identifying the outcome of the question I had about ExcludedNamespaces vs a MatchExpressions is currently blocking any further API review, read through the rest of the enhancement though and only found a couple of things to comment on

@Prashanth684
Copy link
Contributor Author

@JoelSpeed @ingvagabund could you PTAL the updates and let us know if you have more concerns or if all have been addressed?

@bparees
Copy link
Contributor

bparees commented Aug 14, 2023

@Prashanth684 this mostly looks ready for approval, but:

  1. please go through and resolve all the open conversations assuming you have incorporated the feedback into the EP itself (and if not, then the feedback should be added to the EP, not just addressed in the comment thread).

  2. there are a number of questions in the "open questions" section for which the answer seems to have significant implications on delivering the implementation/feature. They may not block you getting to a "phase 1" tech preview type implementation, but they will definitely need to be addressed prior to having a GA-ready solution imho.

e.g.:

Do we need to be aware of any unique ways to access pull secrets for
  disconnected clusters?

and

 Should the `SchedulingGateMutatingWebhook` and the `PodPlacementConfig` CR also consider a Pod Selector to filter
  pods that should be gated and inspected?

@Prashanth684
Copy link
Contributor Author

@Prashanth684 this mostly looks ready for approval, but:

  1. please go through and resolve all the open conversations assuming you have incorporated the feedback into the EP itself (and if not, then the feedback should be added to the EP, not just addressed in the comment thread).
  2. there are a number of questions in the "open questions" section for which the answer seems to have significant implications on delivering the implementation/feature. They may not block you getting to a "phase 1" tech preview type implementation, but they will definitely need to be addressed prior to having a GA-ready solution imho.

e.g.:

Do we need to be aware of any unique ways to access pull secrets for
  disconnected clusters?

and

 Should the `SchedulingGateMutatingWebhook` and the `PodPlacementConfig` CR also consider a Pod Selector to filter
  pods that should be gated and inspected?

@bparees most open conversations have been resolved except for the one about the deletion of the CR which I believe can be resolved as well.

We went through the open questions section and updated it to remove some of them which we have found to not be necessary - for example the one you highlighted around investigating if disconnected installs have a special way of accessing pull secrets. After experimenting with the poc code, we found that to not be the case. The questions in the open questions section relate mainly to performance which we will only find as we experiment with the feature and collect data as it is being used. I believe there are no "blocker" questions which still need to be addressed.

As for

Should the `SchedulingGateMutatingWebhook` and the `PodPlacementConfig` CR also consider a Pod Selector to filter
   pods that should be gated and inspected?

I don't think we need this in the open questions as it can be seen as an "add-on" to the base feature which we can consider when the feature evolves and if we see a need when users start adopting it.

@bparees
Copy link
Contributor

bparees commented Aug 16, 2023

All discussions appear to be resolved with the only remaining objection/question being about the global secrets permission requirement. I don't think there's a way for this to move forward w/o read permission on global secrets, so i think we need to accept that, but the fact that the operator will have those permissions definitely needs to be taken into consideration during implementation to ensure the permission cannot be abused by users to access/use secrets they can't normally use(including using this operator to fetch metadata about images they do not have access to themselves)

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 16, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees

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 Aug 16, 2023
@JoelSpeed
Copy link
Contributor

/lgtm
/hold are we waiting for anyone else to approve this before merge?

@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 Aug 17, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 17, 2023
@Prashanth684
Copy link
Contributor Author

/lgtm /hold are we waiting for anyone else to approve this before merge?

the only other person i'd like to take a look is @joelanford from the OLM perspective to see if there are any concerns (I don't expect any). I've asked him to take a look.

@joelanford
Copy link
Member

Left a few comments, but nothing blocking.

/lgtm
from an OLM perspective.

This enhancement proposes a multiarch manager operator which will be a
secondary OLM managed operator installable through OperatorHub. The
operator will expose functionality which when configured, would allow
pods to be scheduled on the appropriate architecture nodes based on the
architecture of the container images.

The name of the operator is chosen as such in case future functionality
would need to be added that relates to multi-architectural workings.

xref: https://issues.redhat.com/browse/MIXEDARCH-215

Co-authored-by: Alessandro Di Stefano <aleskandro@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 21, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 21, 2023

@Prashanth684: 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/test-infra repository. I understand the commands that are listed here.

@joelanford
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 21, 2023
@Prashanth684
Copy link
Contributor Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 21, 2023
@openshift-merge-robot openshift-merge-robot merged commit b19db47 into openshift:master Aug 21, 2023
2 checks passed
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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet