Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

Kubefed controller pod level annotations #1465

Closed
wants to merge 0 commits into from
Closed

Kubefed controller pod level annotations #1465

wants to merge 0 commits into from

Conversation

nitinatgh
Copy link
Contributor

Added in description to the README.md
Bumped up the chart version of the controller manager
Bumped up the dependency version

Added a new annotation and left the existing one incase it breaks any other previous changes. But not sure if the previous one created was done correctly, so have left it intact.

What this PR does / why we need it: To have annotations set at the pod level for the kubefed controller. Currently it's only restricted to the deployment level.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1447

Special notes for your reviewer: I have created a new annotation as the previous one committed seems to be incorrect and I didn't want to have any breaking changes related to it.

I have tested the changes in a local kind cluster, below is the output from running the helm upgrades

Added the following in the values.yaml

...
  imagePullSecrets: []
  # - name: secretName

  controller:
    annotations: {}
    podAnnotations:
      ad.datadoghq.com/controller-manager.check_names: |
        ["openmetrics"]
      ad.datadoghq.com/controller-manager.init_configs: |
        [{}]
      ad.datadoghq.com/controller-manager.instances: |
        [
          {
            "prometheus_url": "http://%%host%%:%%port%%/metrics",
            "namespace": "kube-federation-system",
            "metrics": [ "*" ],
            "send_distribution_buckets": true,
            "send_distribution_counts_as_monotonic": true,
            "send_histograms_buckets": true,
            "send_monotonic_counter": true
          }
        ]
    replicaCount: 2
    repository: quay.io/kubernetes-multicluster
    image: kubefed
...

helm upgrade dry-run

helm --namespace kube-federation-system upgrade -i kubefed ./kubefed --create-namespace --dry-run

...
# Source: kubefed/charts/controllermanager/templates/deployments.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: kubefed-controller-manager
  namespace: kube-federation-system
  labels:
    kubefed-control-plane: controller-manager
spec:
  replicas: 2
  selector:
    matchLabels:
      kubefed-control-plane: controller-manager
  strategy: {}
  template:
    metadata:
      labels:
        kubefed-control-plane: controller-manager
      annotations:
        ad.datadoghq.com/controller-manager.check_names: |
          ["openmetrics"]
        ad.datadoghq.com/controller-manager.init_configs: |
          [{}]
        ad.datadoghq.com/controller-manager.instances: |
          [
            {
              "prometheus_url": "http://%%host%%:%%port%%/metrics",
              "namespace": "kube-federation-system",
              "metrics": [ "*" ],
              "send_distribution_buckets": true,
              "send_distribution_counts_as_monotonic": true,
              "send_histograms_buckets": true,
              "send_monotonic_counter": true
            }
          ]
    spec:
      nodeSelector:
        {}
      tolerations:
...

helm upgrade

$ kubectl get pods -A

NAMESPACE                NAME                                             READY   STATUS    RESTARTS   AGE
kube-federation-system   kubefed-admission-webhook-67fb48d5f7-hhjhm       1/1     Running   0          23h
kube-federation-system   kubefed-controller-manager-76c659dd6b-924tp      1/1     Running   0          11s
kube-federation-system   kubefed-controller-manager-76c659dd6b-rq27r      1/1     Running   0          13s
kube-system              coredns-558bd4d5db-g29mw                         1/1     Running   0          24h
kube-system              coredns-558bd4d5db-rfggp                         1/1     Running   0          24h
kube-system              etcd-cluster1-control-plane                      1/1     Running   0          24h

$ kubectl describe pod -n kube-federation-system  kubefed-controller-manager-76c659dd6b-924tp

Name:         kubefed-controller-manager-76c659dd6b-924tp
Namespace:    kube-federation-system
Priority:     0
Node:         cluster1-control-plane/172.18.0.2
Start Time:   Wed, 25 Aug 2021 13:55:07 +0100
Labels:       kubefed-control-plane=controller-manager
              pod-template-hash=76c659dd6b
Annotations:  ad.datadoghq.com/controller-manager.check_names: ["openmetrics"]
              ad.datadoghq.com/controller-manager.init_configs: [{}]
              ad.datadoghq.com/controller-manager.instances:
                [
                  {
                    "prometheus_url": "http://%%host%%:%%port%%/metrics",
                    "namespace": "kube-federation-system",
                    "metrics": [ "*" ],
                    "send_distribution_buckets": true,
                    "send_distribution_counts_as_monotonic": true,
                    "send_histograms_buckets": true,
                    "send_monotonic_counter": true
                  }
                ]
              kubectl.kubernetes.io/restartedAt: 2021-08-25T12:55:04Z
Status:       Running
IP:           10.244.0.64
IPs:
  IP:           10.244.0.64
...

Also would like to give a special thanks to https://github.com/kiich for helping me out on this change!

Thanks

Nitin

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 25, 2021
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 25, 2021
@nitinatgh
Copy link
Contributor Author

This PR replaces #1453 due to branch issues.
@yuswift @makkes @xunpan
Apologies again for this.

@xunpan
Copy link
Contributor

xunpan commented Oct 25, 2021

@nitinatgh
Thanks for updating. The code is fine to me. Please squash 3 commits into 1 commit to keep the commit history clean.

@makkes
Copy link
Contributor

makkes commented Oct 25, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2021
@nitinatgh
Copy link
Contributor Author

@nitinatgh Thanks for updating. The code is fine to me. Please squash 3 commits into 1 commit to keep the commit history clean.

Hi @xunpan ,

I'm in a bit of a situation to do that, I have committed directly to my forked master so I don't have the option to squash it, can it not be squashed at the merge stage once approved?

Otherwise i'll need to start everything again from scratch? Sorry about this.

Thanks

Nitin

@xunpan
Copy link
Contributor

xunpan commented Oct 27, 2021

@nitinatgh

I can only approve the PR but cannot merge it.

Could please you have a look at:
https://stackoverflow.com/questions/5189560/squash-my-last-x-commits-together-using-git

@k8s-ci-robot k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 27, 2021
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@nitinatgh nitinatgh closed this Oct 27, 2021
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 27, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nitinatgh

The full list of commands accepted by this bot can be found 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

@nitinatgh
Copy link
Contributor Author

Sorry @xunpan , after trying to rebase it made it worse, and then I reverted the changes on my fork master and it automatically closed this PR.
Will start all over again and it properly now. Sorry yet again.

@xunpan
Copy link
Contributor

xunpan commented Oct 28, 2021

@nitinatgh
Hi, I write a gist to show the related command usage in my work.
FYI: https://gist.github.com/xunpan/010cfa473788b296a435c396519525ed

@makkes
Copy link
Contributor

makkes commented Oct 28, 2021

Will start all over again and it properly now. Sorry yet again.

Dear @nitinatgh, thank you for trying to contribute to kubefed. I understand that all the organizational requirements around PRs on this repo might come across as confusing to you but I would like to ask two things from you before you create the fourth (after #1448, #1453 and this one) PR for the very same change:

  1. Please read and understand the CONTRIBUTING guidelines
  2. Please get familiar with the basics of git, including branching, rebasing and commit squashing. A good starting point might be https://docs.github.com/en/get-started/quickstart/set-up-git#using-git.

@nitinatgh
Copy link
Contributor Author

Thanks @xunpan @makkes for the feedback and also been patient and understanding.

@nitinatgh
Copy link
Contributor Author

New PR created, hopefully this is the one!
#1468
@xunpan @makkes

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add custom annotations to pod level at deployment
4 participants