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.

Sign up for GitHub

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

Jump to bottom

Apparent typo in annotation name #17

Closed
plwhite opened this issue Dec 19, 2019 · 6 comments · Fixed by #18
Closed

Apparent typo in annotation name #17

plwhite opened this issue Dec 19, 2019 · 6 comments · Fixed by #18

Comments

@plwhite
Copy link
Member

plwhite commented Dec 19, 2019

What happend:

Created some resources using multus and multiple networks. Annotations added to pods did not match spec.

What you expected to happen:

According to the spec (https://docs.google.com/document/d/1Ny03h6IDVy_e_vmElOqR7UdTPAG_RNydhVE1Kx54kFQ/) this should lead to an annotation k8s.v1.cni.cncf.io/network-status, but it appeared that the annotation I got was k8s.v1.cni.cncf.io/networks-status (note the extra s to make it networks, not network).

That appears to be a simple typo in the code.
@plwhite plwhite mentioned this issue Dec 19, 2019
Closed
@phoracek
Copy link
Member

CC @s1061123

@phoracek
Copy link
Member

@plwhite thanks for reporting this. This repo follows the implementation from intel/multus-cni. I think it would be less disruptive to fix the document. Could you also raise it there?

@plwhite plwhite mentioned this issue Dec 19, 2019
Open
@s1061123
Copy link
Member

Thank you for the issue, @plwhite !
As far as I looked the code, 'networks-status' is used from initial commit of network status, so let's discuss about it in NPWG meeting.
k8snetworkplumbingwg/multus-cni@8bb0951

@plwhite
Copy link
Member Author

plwhite commented Dec 19, 2019

Fair enough - my instinct is just to change the spec now for consistency with the code.

@s1061123
Copy link
Member

Folks,

I investigated the current implementations (as far as I know, only in Open Sourced) of NPWG Spec.
For now, tungsten(i.e. contrail) and CNI-Genie are using k8s.v1.cni.cncf.io/network-status.

So I suppose chaning NPWG lib (and multus) is the easy way than changing others.
Roughly I come up with the roadmap to fix the issue.

  1. Add k8s.v1.cni.cncf.io/network-status into NPWG lib. During transitional phase, k8s.v1.cni.cncf.io/networks-status keeps in NPWG lib until next NPWG spec version-up
  2. At the NPWG spec's next version-up, NPWG lib removes k8s.v1.cni.cncf.io/networks-status

How about that? Comments and questions are welcome.
If no objection, I will work on that.

@plwhite
Copy link
Member Author

plwhite commented Jan 21, 2020

Works for me, thanks.

s1061123 added a commit to s1061123/network-attachment-definition-client that referenced this issue Jan 27, 2020
@s1061123
Fix status annotation name
73b96ca 
This change fixes k8snetworkplumbingwg#17, mismatching status annotation name between
implementation and specification. During transitional period
(i.e. until next version up), old status annotation is kept for
backward compatibility.
@s1061123 s1061123 mentioned this issue Jan 27, 2020
Merged
s1061123 added a commit to s1061123/network-attachment-definition-client that referenced this issue Jan 27, 2020
@s1061123
Fix status annotation name
9c26d1d 
This change fixes k8snetworkplumbingwg#17, mismatching status annotation name between
implementation and specification. During transitional period
(i.e. until next version up), old status annotation is kept for
backward compatibility.
@danwinship danwinship mentioned this issue Aug 27, 2020
Merged
@innobead innobead mentioned this issue Oct 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix status annotation name s1061123/network-attachment-definition-client
3 participants
@s1061123 @phoracek @plwhite