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

Create v2 of node distribution standard (issues/#494) #524

Merged
merged 12 commits into from
Jun 17, 2024

Conversation

cah-hbaum
Copy link
Contributor

Adds the new label topology.scs.openstack.org/host-id to the standard and extend the standard to require providers to set the labels on their managed k8s clusters.

@cah-hbaum cah-hbaum added Container Issues or pull requests relevant for Team 2: Container Infra and Tooling standards Issues / ADR / pull requests relevant for standardization & certification SCS is standardized SCS is standardized SCS-VP10 Related to tender lot SCS-VP10 labels Mar 15, 2024
@cah-hbaum cah-hbaum self-assigned this Mar 15, 2024
Copy link
Member

@matofeder matofeder left a comment

Choose a reason for hiding this comment

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

LGTM

@mbuechse mbuechse linked an issue Mar 18, 2024 that may be closed by this pull request
3 tasks
@mbuechse mbuechse changed the title Update node distribution standard (issues/#540) Update node distribution standard (issues/#494) Mar 18, 2024
@mbuechse
Copy link
Contributor

Let Team Container in its next meeting vote on whether this needs a v2; apart from that: good to go!

@mbuechse
Copy link
Contributor

@cah-hbaum you are quick! Let's add a paragraph that explains the changes vs v1.

@cah-hbaum
Copy link
Contributor Author

@mbuechse Sure thing. Gonna need to do something else first, but after that I'm on it.

Copy link
Member

@martinmo martinmo left a comment

Choose a reason for hiding this comment

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

I agree, this was really quick! Very good. I have left some review comments we should discuss about, otherwise LGTM.

Standards/scs-0214-v2-k8s-node-distribution.md Outdated Show resolved Hide resolved
Standards/scs-0214-v2-k8s-node-distribution.md Outdated Show resolved Hide resolved
Standards/scs-0214-v2-k8s-node-distribution.md Outdated Show resolved Hide resolved
@martinmo
Copy link
Member

@cah-hbaum I forgot to mention that the title and description of this PR should be updated to reflect that we create a new version v2.

@mbuechse mbuechse changed the title Update node distribution standard (issues/#494) Create v2 of node distribution standard (issues/#494) Mar 21, 2024
@mbuechse
Copy link
Contributor

@cah-hbaum @martinmo @matofeder Please be careful before merging. This document will be merged in stable state, so let's be extra certain that it is good this time... I pushed the date to April 4, so there's no need to hurry.

@cah-hbaum
Copy link
Contributor Author

This should be done now. After some more approvals fly in, I will merge this.

Copy link
Member

@martinmo martinmo left a comment

Choose a reason for hiding this comment

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

Just found two typos and a cosmetic issue – apart from that, it is good to go!

Standards/scs-0214-v2-k8s-node-distribution.md Outdated Show resolved Hide resolved
Standards/scs-0214-v2-k8s-node-distribution.md Outdated Show resolved Hide resolved
@cah-hbaum
Copy link
Contributor Author

I changed the document state to Draft now like we discussed in the Standardization/Certification meeting.

I also fixed (probably) all problems that had remained, so it should be mergable now.

@cah-hbaum
Copy link
Contributor Author

@mbuechse Please check this again, since this is in Draft now.

@cah-hbaum cah-hbaum requested a review from mbuechse April 18, 2024 08:51
@cah-hbaum cah-hbaum force-pushed the issue/issues/540-update-node-distribution-standard branch 4 times, most recently from c265f73 to 6577a58 Compare June 5, 2024 11:45
@cah-hbaum
Copy link
Contributor Author

@artificial-intelligence I added you here for review, because we had the question, if this standard is possible to do with a Gardener setup and @mbuechse mentioned that you have some knowledge here. If that is not the case, you can "unrequest" yourself and maybe put in someone else who has more knowledge about this.

@cah-hbaum
Copy link
Contributor Author

Also because we talked about this in the meeting:
This standard is kind of "blocked" right now, since neither the reference implementation nor a few other K8s examples automatically provide the labels mentioned here.
BUT the standard could be merged nonetheless, since it is only in Draft for now.

Copy link
Member

@chess-knight chess-knight left a comment

Choose a reason for hiding this comment

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

Draft approved

Comment on lines +69 to +73
The control plane nodes MUST be distributed over multiple physical machines.
Kubernetes provides [best-practices][k8s-zones] on this topic, which are also RECOMMENDED by SCS.

At least one control plane instance MUST be run in each "failure zone" used for the cluster,
more instances per "failure zone" are possible to provide fault-tolerance inside a zone.
Copy link
Member

Choose a reason for hiding this comment

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

I experienced an interesting scenario, where the node distribution test will succeed.
SCS Compliance results in SovereignCloudStack/k8s-cluster-api-provider#742 (comment) show INFO: The nodes are distributed across 2 host-ids.
I am wondering now if 3 control plane nodes are distributed over 2 hosts, it means that 2 control plane nodes are located on the same physical host. What will happen with the k8s cluster if this host goes down? AFAIK etcd has in this case only one member failure tolerance, it will stop functioning!

Copy link
Member

Choose a reason for hiding this comment

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

These lines in the standard and corresponding test don't require hard anti-affinity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand the concern. We would probably need to adapt the lines above the ones you mentioned from

The control plane nodes MUST be distributed over multiple physical machines.
Kubernetes provides [best-practices][k8s-zones] on this topic, which are also RECOMMENDED by SCS.

to

The control plane nodes MUST be distributed over multiple physical machines; a control plane MUST contain at least three nodes and be distributed over three machines.
Kubernetes provides [best-practices][k8s-zones] on this topic, which are also RECOMMENDED by SCS.

Would that be alright?

Copy link
Member

Choose a reason for hiding this comment

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

at least three nodes can this be considered as node distribution standard or is it more suited for some k8s HA standard?

Copy link
Member

Choose a reason for hiding this comment

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

Also, can we somehow incorporate the fact that etcd can be external? Then we are probably good with the current formulation, this comment is valid only if stacked etcd for k8s cluster is used.

Copy link
Contributor Author

@cah-hbaum cah-hbaum Jun 17, 2024

Choose a reason for hiding this comment

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

Probably more relevant for a HA standard, but TBF that was always the problem I had with this standard in general, since node distribution or better fault tolerance also kind of (at least partly) includes (high) availability.
Personally, after working on this standard this long I would probably change it up altogether to create a (high) availability that is based on this document and make this distribution standard optional, since not every cluster needs to be distributed (IMO).

Coming back to the case presented by you above:

I am wondering now if 3 control plane nodes are distributed over 2 hosts, it means that 2 control plane nodes
are located on the same physical host. What will happen with the k8s cluster if this host goes down?
AFAIK etcd has in this case only one member failure tolerance, it will stop functioning!

I understand this scenario (and as far as I know you're right about the etcd functionality). The question would be how to fix this scenario in the standard. There shouldn't be a number of physical nodes smaller than three, if etcd in use has 3 members.
So I guess this means this is still a distribution issue for us and not a HA issue?

Copy link
Member

Choose a reason for hiding this comment

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

Also, can we somehow incorporate the fact that etcd can be external? Then we are probably good with the current formulation, this comment is valid only if stacked etcd for k8s cluster is used.

Now I am reading that also for external etcd you need 3+3 nodes. Do you know why it is so?

I understand this scenario (and as far as I know you're right about the etcd functionality). The question would be how to fix this scenario in the standard. There shouldn't be a number of physical nodes smaller than three, if etcd in use has 3 members.
So I guess this means this is still a distribution issue for us and not a HA issue?

https://kubernetes.io/docs/setup/best-practices/multiple-zones/#control-plane-behavior mentions:

If availability is an important concern, select at least three failure zones and replicate each individual control plane component (API server, scheduler, etcd, cluster controller manager) across at least three failure zones.

So maybe we can keep it and put it only later to the HA standard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, I already read about this some time ago.
I guess the idea behind the external etcd is separation of concerns in your infrastructure and better control (and possible security) for a cluster. So it makes sense to separate the underlying etcd nodes and the kubernetes control plane. As the linked article mentions

This topology decouples the control plane and etcd member. It therefore provides an HA setup where losing a
control plane instance or an etcd member has less impact and does not affect the cluster redundancy
as much as the stacked HA topology.

Now I think that use-case wouldn't be implemented that often, but its an interesting thing to consider for a setup with required redundancy.

Personally, I would put it into a future High availability standard.

I will mention all this in #639 and we can keep on discussing everything there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrote and mentioned most of the stuff in the description of #639

cah-hbaum and others added 12 commits June 17, 2024 11:01
Adds the new label topology.scs.openstack.org/host-id to the standard and extend the standard to require providers to set the labels on their managed k8s clusters.

Signed-off-by: Hannes Baum <[email protected]>
Like discussed in the SIG Standardization and Certification Meeting, the standard is updated to v2 with the changes made to it and immediately set to "Stable", since no major changes were done. This comes mainly from the rushed job we did.

Signed-off-by: Hannes Baum <[email protected]>
Signed-off-by: Matthias Büchse <[email protected]>
Signed-off-by: Matthias Büchse <[email protected]>
Signed-off-by: Matthias Büchse <[email protected]>
Signed-off-by: Matthias Büchse <[email protected]>
Like it was discussed in the Standardization/Certification meeting, we will set this v2 to a Draft state for now.

Signed-off-by: Hannes Baum <[email protected]>
Some small cosmetic updates fixing some problems mentioned by @martinmo.

Signed-off-by: Hannes Baum <[email protected]>
Did some more textual updates requested by @mbuechse.

Signed-off-by: Hannes Baum <[email protected]>
@cah-hbaum cah-hbaum force-pushed the issue/issues/540-update-node-distribution-standard branch from 6577a58 to 81b9ab0 Compare June 17, 2024 09:14
Copy link
Member

@chess-knight chess-knight left a comment

Choose a reason for hiding this comment

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

I am wondering if this standard is valid also for clusters with the managed control plane. E.g. Kamaji, where CP runs inside pods in the management cluster and when you do kubectl get nodes in the workload cluster you see only worker nodes there.

@cah-hbaum
Copy link
Contributor Author

I am wondering if this standard is valid also for clusters with the managed control plane. E.g. Kamaji, where CP runs inside pods in the management cluster and when you do kubectl get nodes in the workload cluster you see only worker nodes there.

Thats also something we had some discussions about already and we decided that we want to have a discussion about this in the follow up issue that would make this standard Stable.

@cah-hbaum
Copy link
Contributor Author

I'm gonna keep it like it is for now and will update the standard in the follow-up issue with regards to the things discussed here.

@cah-hbaum cah-hbaum merged commit 6c82357 into main Jun 17, 2024
6 checks passed
@cah-hbaum cah-hbaum deleted the issue/issues/540-update-node-distribution-standard branch June 17, 2024 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Container Issues or pull requests relevant for Team 2: Container Infra and Tooling SCS is standardized SCS is standardized SCS-VP10 Related to tender lot SCS-VP10 standards Issues / ADR / pull requests relevant for standardization & certification
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Standard] Node distribution v2
5 participants