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

Add Node affinity #1003

Merged
merged 4 commits into from
Oct 26, 2023
Merged

Add Node affinity #1003

merged 4 commits into from
Oct 26, 2023

Conversation

bdunne
Copy link
Member

@bdunne bdunne commented Oct 17, 2023

For OpenShift 4.14 support

Part of #988

@bdunne
Copy link
Member Author

bdunne commented Oct 17, 2023

@nasark Please also review. The refactoring was inspired by your work in #993. Do you think the miqutils code should continue to live in api/v1alpha1 or should create a api/v1 for it?

@nasark
Copy link
Member

nasark commented Oct 18, 2023

@nasark Please also review. The refactoring was inspired by your work in #993. Do you think the miqutils code should continue to live in api/v1alpha1 or should create a api/v1 for it?

In my opinion since its technically just a refactor I would say to keep it in v1alpha1. Is there any benefit at the moment to creating a v1 API? We would also need to support multiple versions in the CRD if thats the case

Comment on lines +20 to +23
- key: kubernetes.io/arch
operator: In
values:
- amd64
Copy link
Member

Choose a reason for hiding this comment

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

Do we also want s390x and ppc64le like in downstream?

Copy link
Member

Choose a reason for hiding this comment

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

We don't build those targets in upstream, so no? I'm not sure how this will work in downstream though if we have additional arches.

Copy link
Member

Choose a reason for hiding this comment

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

In downstream we have a separate setNodeAffinity function that adds the other arches to the Kafka CR

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we also want s390x and ppc64le like in downstream?

No, we only build x86_64 upstream

I'm not sure how this will work in downstream though if we have additional arches.

We don't use this file downstream. The downstream operator will deploy the manageiq operator with the correct values.

@bdunne
Copy link
Member Author

bdunne commented Oct 23, 2023

@nasark Please also review. The refactoring was inspired by your work in #993. Do you think the miqutils code should continue to live in api/v1alpha1 or should create a api/v1 for it?

In my opinion since its technically just a refactor I would say to keep it in v1alpha1. Is there any benefit at the moment to creating a v1 API? We would also need to support multiple versions in the CRD if thats the case

  • I was thinking of v1 just for the miqutils, not the CRD and everything else yet. (That's much more involved)
  • v1 implies stable library, but maybe this isn't there yet.

func OperatorNodeAffinityArchValues(deployment *appsv1.Deployment, client client.Client) []string {
podName := os.Getenv("POD_NAME")
pod := FindPodByName(client, deployment.ObjectMeta.Namespace, podName)
values := []string{"amd64"}
Copy link
Member Author

Choose a reason for hiding this comment

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

I added a commit to default to amd64

Copy link
Member

@nasark nasark left a comment

Choose a reason for hiding this comment

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

LGTM, just a note that Strimzi Kafka will require a different method of setting node affinity (similar to downstream). But that can be handled in #1005 I suppose

Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

LGTM, also want @nasark approval before merge.

@Fryguy
Copy link
Member

Fryguy commented Oct 24, 2023

@bdunne How do we set the node affinities for the workloads? (workers and now also the automaton worker's jobs?)

I also updated the OP to say this is part of #988, but wasn't sure if it "completed" #988 or if we need other PRs (such as the node affinities for the workloads)

@bdunne
Copy link
Member Author

bdunne commented Oct 26, 2023

@bdunne How do we set the node affinities for the workloads? (workers and now also the automaton worker's jobs?)

I also updated the OP to say this is part of #988, but wasn't sure if it "completed" #988 or if we need other PRs (such as the node affinities for the workloads)

This will discover the node affinities applied to the operator and apply the same affinities to httpd, postgres, memcached and the orchestrator. I'll be opening a PR shortly that does the same tihng inside the orchestrator for the deployments that it creates.

@Fryguy
Copy link
Member

Fryguy commented Oct 26, 2023

@bdunne Can we merge this now or do we have to wait for the core orchestrator change?

Second question. If we run on an older OpenShift, will these nodeAffinities cause issues, or are they just ignored on older OpenShifts

@bdunne
Copy link
Member Author

bdunne commented Oct 26, 2023

This can be merged before the orchestrator changes.

My test cluster is 4.13.4 and it's working fine there. I can't find when kubernetes.io/arch label was introduced, but I see references to it over 3 years ago.

@Fryguy
Copy link
Member

Fryguy commented Oct 27, 2023

Skipping backport to quinteros, because it is already in the branch.

@Fryguy
Copy link
Member

Fryguy commented Oct 31, 2023

Skipping backport to quinteros, because it is already in the branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants