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

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

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.

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 <hannes.baum@cloudandheat.com>
@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
Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>
Update the label topology.scs.openstack.org/host-id in the node distribution test to topology.scs.community/host-id.

Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>
Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>
Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>
Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>
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

Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>
Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>
Signed-off-by: Matthias Büchse <matthias.buechse@cloudandheat.com>
Signed-off-by: Matthias Büchse <matthias.buechse@cloudandheat.com>
@mbuechse mbuechse linked an issue Mar 18, 2024 that may be closed by this pull request
2 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!

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 <hannes.baum@cloudandheat.com>
mbuechse and others added 4 commits March 22, 2024 16:13
Signed-off-by: Matthias Büchse <matthias.buechse@cloudandheat.com>
Signed-off-by: Matthias Büchse <matthias.buechse@cloudandheat.com>
Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>
Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>
@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
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 <hannes.baum@cloudandheat.com>
Some small cosmetic updates fixing some problems mentioned by @martinmo.

Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>
Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>
@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
Copy link
Contributor

@mbuechse mbuechse left a comment

Choose a reason for hiding this comment

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

As discussed yesterday in SIG std/cert, we still want to refine a few things.

Comment on lines 68 to 70
Since some providers only have small environments to work with and therefore couldn't
comply with this standard, it will be treated as a RECOMMENDED standard, where providers
can OPT OUT.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would drop this sentence. We should, however, provide implementation notes.

And we should clarify with the help of Team Container what really happens with Gardener (and the like): if there is a set of control-plane nodes that are being shared by the control planes of multiple clusters -- is it really the case that these nodes cannot be enumerated? Can we maybe get access to such a cluster (I think @berendt is working on that right now and we should get access soon)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, why is this sentence still in here? Have you found out in the meantime (with the help of Team Container) what the situation is like with a shared pool of control-plane nodes, and how this would present itself to the test?

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
Standards/scs-0214-v2-k8s-node-distribution.md Outdated Show resolved Hide resolved
Comment on lines 83 to 85
- To provide metadata about the node distribution, which also enables testing of this standard,
providers MUST annotate their K8s nodes with the labels listed below. These labels MUST be kept
up to date with the current state of the deployment.
Copy link
Contributor

Choose a reason for hiding this comment

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

As of now, we don't have any idea when this can be implemented with CAPO. @matofeder Do you have an update?
Unless the implemenation is in sight, we should demote this from a "MUST annotate" to a "SHOULD annotate". (The "MUST be kept up to date" can remain, so long as it's clear that it only applies if labels are being set at all.)

Copy link
Member

Choose a reason for hiding this comment

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

@chess-knight as you have better visibility of this topic now, please provide an update here, thx

Copy link
Member

Choose a reason for hiding this comment

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

The draft PR SovereignCloudStack/cluster-stacks#53 with hostId labeling workaround was created by @matofeder and @SovereignCloudStack/vp06c will continue that work ASAP.
Regarding upstream, there is no progress AFAIK.

Copy link
Member

Choose a reason for hiding this comment

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

I created an upstream issue kubernetes/cloud-provider-openstack#2579 so let's wait for their opinion on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this seems to be a low-hanging fruit in the opinion of the upstream guys, should I/we put the code in ourselves in order to speed the process up (kubernetes/cloud-provider-openstack#2579) ?

Copy link
Member

Choose a reason for hiding this comment

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

PR SovereignCloudStack/cluster-stacks#53 is ready for review and it includes a workaround (creating server groups, instructing CAPO to use these server groups for nodes, labelling nodes with custom host-id label). kubernetes/cloud-provider-openstack#2579 is about labelling part and @jschoone wanted to contact them. The server group part is still missing in CAPO. I will try to comment in kubernetes-sigs/cluster-api-provider-openstack#1912 to speed it up.

Copy link
Member

Choose a reason for hiding this comment

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

Also, I am unsure if it is low-hanging fruit, as I wrote in kubernetes/cloud-provider-openstack#2579 - Live migration can be problematic because the label will not be updated. IMO, it needs a discussion.


And what about bare-metal clusters? In that case, we do not need this specific label right?

Did some more textual updates requested by @mbuechse.

Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>
Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>
@cah-hbaum cah-hbaum requested a review from mbuechse April 30, 2024 10:12
Copy link
Contributor

@mbuechse mbuechse left a comment

Choose a reason for hiding this comment

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

Fine apart from the change that I indicated.

Standards/scs-0214-v2-k8s-node-distribution.md Outdated Show resolved Hide resolved
Comment on lines 68 to 70
Since some providers only have small environments to work with and therefore couldn't
comply with this standard, it will be treated as a RECOMMENDED standard, where providers
can OPT OUT.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, why is this sentence still in here? Have you found out in the meantime (with the help of Team Container) what the situation is like with a shared pool of control-plane nodes, and how this would present itself to the test?

Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>
@cah-hbaum
Copy link
Contributor Author

Wait, why is this sentence still in here? Have you found out in the meantime (with the help of Team Container) what
the situation is like with a shared pool of control-plane nodes, and how this would present itself to the test?

No, I haven't (because I had two other meetings conflicting with the last container meeting).

My problem cutting this part would still be that smaller CSPs can't necessarily provide a setup like this, especially for all their clusters. And that probably isn't necessary either.
IMO what would be needed here is a requirement for this standard to be used. Now I wouldn't call such a setup "high availability" necessarily, but something in that direction would be nice to have.

@cah-hbaum cah-hbaum requested a review from mbuechse May 22, 2024 07:11
@cah-hbaum
Copy link
Contributor Author

@mbuechse Please check this again and resolve the parts that are fine with you. I'm gonna coordinate the upstream work with @chess-knight now.

@mbuechse
Copy link
Contributor

mbuechse commented May 23, 2024

@cah-hbaum Looking good, thanks for all the work! Now, only the matter of the "optionality" of this standard needs some discussion IMO. The certificate scope yaml permits marking a standard optional, but then I don't know what good the standard is, because most people, when they look at the certificate, they will never notice whether this particular standard was satisfied or not. And I still don't understand why this standard is so hard to satisfy after all, even for small providers. If it's indeed possible that multiple k8s clusters share the same hosts for their control plane, then each cluster can have three different hosts, and all clusters in total still only need three hosts, so where's the problem?

@cah-hbaum
Copy link
Contributor Author

@cah-hbaum Looking good, thanks for all the work! Now, only the matter of the "optionality" of this standard needs some discussion IMO. The certificate scope yaml permits marking a standard optional, but then I don't know what good the standard is, because most people, when they look at the certificate, they will never notice whether this particular standard was satisfied or not. And I still don't understand why this standard is so hard to satisfy after all, even for small providers. If it's indeed possible that multiple k8s clusters share the same hosts for their control plane, then each cluster can have three different hosts, and all clusters in total still only need three hosts, so where's the problem?

I'm gonna update the standard to make it NOT optional and take it to a vote or request in the "SCS Container" Room.

Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>
@cah-hbaum
Copy link
Contributor Author

Posted into the "SCS Container" Matrix room, just putting the whole thing in here as a paraphrase to make it persistent:

Hello again,

I got some standardization work again for you today and a request for some input for #524.
This PR updates the "Node Distribution" standard. Before the latest update, the standard was OPTIONAL for a CSP, since
we also had small CSPs with not much available infrastructure in mind. Sure there are possible solutions for these
scenarios (e.g. solutions with shared control-planes to make the overall footprint of clusters smaller allegidly), but a
change to these solutions isn't always possible.

Now while we tried to figure these problems out, some more potential points for problems appeared: #527
With these failsafe levels, some things we probably need to talk about in the context of this standards come up, like
(High) Availability, Failsafe, Failover, Redundancy and so on.

Personally, at this point in time, I would probably rework the standard in a broader fashion. I don't think that we can or
should dictate this "Node Distribution" (which is kind of a "Redundancy"/"Availability" feature) for every K8s cluster that
is offered through a KaaS offering.
Instead, the standard should define the Use Case it wishes to accomplish/help with, which is probably some kind of
"(High) Availability", and make this (at least a bit) dependent on the now developing "Failsafe Taxonomy" (#527).
We could then define the requirements (in this case the "Node Distribution") for such an offering. The standard should
probably made broarder in this case, since "Availability" in K8s requires more than just distribution of nodes. But the
currently used document could become a part of this future document.

Now I would like to think what your opinion about this is or if you would like this to go in another direction?

Thanks and best regards
Hannes

@cah-hbaum
Copy link
Contributor Author

Another thing we also maybe need to discuss came up while working on #556. I installed a K8s cluster through yaook/k8s and ran into the problem, that the labels required by this standard were not available. This is kind of obvious, since we don't use the preexisting solutions we expected with this standard.
But I'm also a bit doubtful about the way we did checking with this standard / test now. We can't really assure that the necessary labels are available for every solution that could possibly be used for a possibly SCS-compatible cluster.

In my case, is the cluster really "not distributed" just because the labels aren't there; the values even were available just under different labels.
TBF I don't have a good solution here, just wanted to ask about our handling of this matter.

(Labels default in yaook/k8s are kubernetes.io/hostname=XXX and topology.cinder.csi.openstack.org/zone=YYY, a region wasn't available)

@martinmo
Copy link
Member

IMHO it is ok to require conformant implementations to set some additional labels, even if some work needs to be done initially to support it (e.g., in yaook/k8s). And especially with topology.kubernetes.io/region and topology.kubernetes.io/zone, we have some well-known labels defined upstream by Kubernetes (see https://kubernetes.io/docs/reference/labels-annotations-taints) – basically, they are already standardized, but optional.

Standardized labels are not only useful for the conformance test. Another important use case is that the customers can use these labels to distribute their own applications over failure zones using Pod topology spread constraints.

Also, in #524 (comment), an important question about topology.scs.community/host-id was raised:

And what about bare-metal clusters? In that case, we do not need this specific label right?

Theoretically, in a bare-metal cluster, the kubernetes.io/hostname label would be enough. However, the conformance test would need to detect this situation and correctly decide which label (host-id or hostname) should be used. This is an additonal source of error. I think even a bare metal K8s node should have a host-id label (every Linux machine should have it, accessible via hostid or in binary form at /etc/hostid).

@mbuechse
Copy link
Contributor

I think even a bare metal K8s node should have a host-id label (every Linux machine should have it, accessible via hostid or in binary form at /etc/hostid).

Yes, that would be desireable from my POV. People shouldn't be expected to check whether the cluster is running on bare metal or not.

@martinmo
Copy link
Member

(Labels default in yaook/k8s are kubernetes.io/hostname=XXX and topology.cinder.csi.openstack.org/zone=YYY, a region wasn't available)

I double checked this: yaook/k8s connects to the OpenStack layer via the OpenStack cloud controller manager (CCM) which sets the topology.kubernetes.io/region and topology.kubernetes.io/zone labels, amongst others. But it requires that the underlying OpenStack instance has the corresponding metadata set (in other words, it requires proper AZ and region configuration at the IaaS layer).

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: Doing
Development

Successfully merging this pull request may close these issues.

[Standard] Node distribution v2
5 participants