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

Adding Status field to NetworkAttachmentDefinition #38

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nishantsankhala
Copy link

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
Copy link
Member

s1061123 commented May 21, 2021

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
Copy link

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
Copy link
Member

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
Copy link
Member

dougbtv commented May 27, 2021

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
Copy link

michaelhenkel commented May 28, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants