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

typha and namespace adjustments for AKS #1166

Merged
merged 3 commits into from
Feb 18, 2021

Conversation

ozdanborne
Copy link
Member

@ozdanborne ozdanborne commented Feb 17, 2021

Description

Three changes included in this PR, broken down by commit:

  1. adjust typha horizontal autoscaler logic to not count aks 'virtual-nodes' towards the count because:

    1. typha cannot run on them due to hostNetworking
    2. calico-node isn't run on them due to hostNetworking and as such we won't see increased load on k8s-apiserver due to calico-node because of the virtual-node.
  2. configure typha to have anti-affinity to virtual-node because it will fail to run there since it uses hostNetwork which isn't supported on virtual nodes.

  3. add the label control-plane=true to all namespaces as AKS uses this to prevent mutating webhooks from adjusting pods running there.

A couple bits of collateral included in this PR to be aware of:

  1. this updates kibana namespace creation to correctly apply openshift labels if on openshift. previously, isOpenshift was accidentally hardcoded to false (https://github.com/tigera/operator/pull/1166/files#diff-60a1f183b7b6ca2f0ed047c33c6f80c1ed20dd7b20b5b908d736b237a01ca811L346).
  2. To support going from a boolean to enum, we no longer pass the openshift boolean which originates as the provider auto-detected during startup, and instead uses the provider stored in the installation resource. This was done because startup doesn't include detection for AKS, which is needed for this PR. However, the difference between the two is that the auto-detected values ignore / take precedence over what a user has specified. So though the core controller will notice a difference between the user-specified provider and the detected one, the other controllers will not.

Leaving as a draft while I write up some tests.

For PR author

  • Tests for change.
  • If changing pkg/apis/, run make gen-files
  • If changing versions, run make gen-versions

For PR reviewers

A note for code reviewers - all pull requests must have the following:

  • Milestone set according to targeted release.
  • Appropriate labels:
    • kind/bug if this is a bugfix.
    • kind/enhancement if this is a a new feature.
    • enterprise if this PR applies to Calico Enterprise only.

@ozdanborne ozdanborne marked this pull request as ready for review February 17, 2021 22:46
Values: []string{"virtual-node"},
},
{
Key: "kubernetes.azure.com/cluster",
Copy link
Member

Choose a reason for hiding this comment

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

I'm second guessing including this in the match here. If this Provider was set but nodes did not have kubernetes.azure.com/cluster then there would be no nodes matched. That doesn't seem very helpful. I feel like we'd want the logic to be

if key kubernetes.azure.com/cluster key exists and key type has value virtual-node
then do not match

but this is actually requiring key kubernetes.azure.com/cluster to match.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can achieve the logic that we would want so maybe this should just be the key type is not in virtual-node

Copy link
Member

@tmjd tmjd Feb 17, 2021

Choose a reason for hiding this comment

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

Actually I think we might be able to get the behavior we'd want with

NodeSelectorTerms: []v1.NodeSelectorTerm{
    {
        MatchExpressions: []v1.NodeSelectorRequirement{
            {
                Key:      "kubernetes.azure.com/cluster",
                Operator: v1.NodeSelectorOpDoesNotExist,
            },
        },
    },
    {
        MatchExpressions: []v1.NodeSelectorRequirement{
            {
                Key:      "type",
                Operator: corev1.NodeSelectorOpNotIn,
                Values:   []string{"virtual-node"},
            },
            {
                Key:      "kubernetes.azure.com/cluster",
                Operator: v1.NodeSelectorOpExists,
            },
        },
    },
}

That way any kubernetes.azure.com/cluster nodes that do not have type virtual-node can be selected or any nodes that do not have kuberentes.azure.com/cluster can be selected.

Copy link
Member

Choose a reason for hiding this comment

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

We got confirmation that all AKS nodes will have the kubernetes.azure.com/cluster label so this should be good to go.

Copy link
Member

@tmjd tmjd left a comment

Choose a reason for hiding this comment

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

LGTM

@ozdanborne ozdanborne merged commit 298ab9d into tigera:master Feb 18, 2021
@ozdanborne ozdanborne deleted the aks-updates branch February 18, 2021 01:05
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.

None yet

3 participants