search

k8snetworkplumbingwg / network-attachment-definition-client

A Golang Kubernetes client
Apache License 2.0
12 stars 37 forks source link

Adding Status field to NetworkAttachmentDefinition #38

Open nishantsankhala opened 3 years ago

nishantsankhala commented 3 years ago

This PR adds Status field to NetworkAttachmentDefinition. The Status field contains ReconcilerState which can be used by NetworkAttachmentDefinition controller implementation to provide current State of the object and Observation related to the state.

s1061123 commented 3 years ago

Thank you for your PR, @nishantsankhala

As far as I looked the PR, your change includes net-attach-def CRD itself. We define CRD in multi-net-spec, hence you need to change this spec first to modify the CRD. Don't you mind if you attend the NPWG meeting to start the discussion for that? This meeting agenda is in google doc, so you can add agenda for next meeting.

Regarding your change, 'status' field seems to be only for some controller, which may need to have and it seems to be 'optional' field for other use-case. So I suppose you can use annotations in net-attach-def to keep your value, instead of CRD changes. Annotation field does not require any CRD change and you can add your field as you want.

---
apiVersion: "k8s.cni.cncf.io/v1"
kind: NetworkAttachmentDefinition
metadata:
  name: centos-args-def
  annotations:
    state: xxxx
    observation: yyyy
spec:
  config: '{
            "cniVersion": "0.3.1",
            "plugins": [
             (snip)                  
        }'
michaelhenkel commented 3 years ago

I think a proper status field makes sense as it allows to programmatically access the status information in a more kube api native way. Also it doesn't break backwards compatibility. Controllers can use or not use the status field. Existing CRDs would be updated without breaking the behavior.

s1061123 commented 3 years ago

Hi,

As I noted before, your change does not bug fix, enhancement net-attach-def CRD. CRD is defined in multi-net-spec by our community, hence you need to change this spec first to modify the CRD. To change the CRD spec, bi-weekly meeting is the best place to discuss. I guess. Please check the community page for the detail. Looking forward to seeing you at the meeting!

https://github.com/k8snetworkplumbingwg/community

dougbtv commented 3 years ago

I think this is a positive change if I understand it correctly, thanks for the contribution!

However, Tomo is right on, since this object is defined by a working group, we'd like to have a discussion prior to bringing the change in -- just to make sure everyone is on the same page. As Tomo mentioned, the biweekly meeting is the perfect spot. I'll add the discussion to the agenda but it'd be much appreciated if you have the chance to join.

michaelhenkel commented 3 years ago

ack, we'll be there and present

On Wed, May 26, 2021 at 15:19 Tomofumi Hayashi @.***> wrote:

Hi,

As I noted before, your change does not bug fix, enhancement net-attach-def CRD. CRD is defined in multi-net-spec by our community, hence you need to change this spec first to modify the CRD. To change the CRD spec, bi-weekly meeting is the best place to discuss. I guess. Please check the community page for the detail. Looking forward to seeing you at the meeting!

https://github.com/k8snetworkplumbingwg/community

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/k8snetworkplumbingwg/network-attachment-definition-client/pull/38#issuecomment-849158974, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABUFT4LNOUQQQVFK7JQ2OW3TPVXX7ANCNFSM45JGQ2ZQ .