Skip to content

rename network-config.json to ports.json to match reality #4376

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

Closed
wants to merge 2 commits into from

Conversation

AkihiroSuda
Copy link
Member

Also rename networkstore to portstore

Follow-up to PR #4290
cc @haytok

Follow-up to PR 4290

Signed-off-by: Akihiro Suda <[email protected]>
Follow-up to PR 4290

Signed-off-by: Akihiro Suda <[email protected]>
@AkihiroSuda AkihiroSuda added this to the v2.1.3 milestone Jun 26, 2025
@haytok
Copy link
Contributor

haytok commented Jun 26, 2025

Thanks!!! LGTM

@apostasie
Copy link
Contributor

Hey @AkihiroSuda
I am wondering about this.
We are going to need to put more stuff in there soon, as we will want to move more config away from labels to here.
Particularly thinking about multiple IPs for different networks, which will require significant changes.

@apostasie
Copy link
Contributor

apostasie commented Jun 26, 2025

Point being: I think we might want to leave it named network-config.json.

@haytok
Copy link
Contributor

haytok commented Jun 26, 2025

The reason why I named network-config.json and networkstore is that I thought that in the future, we would be able to store information about other network info (e.g. ip adress,hostname, and so on) currently stored in the label as @apostasie commented.

However, if @AkihiroSuda is changing it for a different reason, I think this correction is fine.

@AkihiroSuda
Copy link
Member Author

The current type definition []cni.PortMapping doesn't seem extensible to support non-port config fields?
It has to be changed to a new struct

@AkihiroSuda AkihiroSuda removed this from the v2.1.3 milestone Jun 26, 2025
@AkihiroSuda AkihiroSuda marked this pull request as draft June 26, 2025 16:25
@apostasie
Copy link
Contributor

The current type definition []cni.PortMapping doesn't seem extensible to support non-port config fields? It has to be changed to a new struct

The current type definition []cni.PortMapping doesn't seem extensible to support non-port config fields? It has to be changed to a new struct

@haytok do you want to go for it and change the data structure?

@apostasie
Copy link
Contributor

apostasie commented Jun 26, 2025

Note: failures here are logs related, which have been plaguing us.

Now, while debugging healthcheck failures, I now believe we might have an underlying problem with containerd cio.
See #4377 for a case where (apparently) running a task fails to capture the output.
^ either we do not implement task running correctly, or something is borked inside containerd.

@haytok
Copy link
Contributor

haytok commented Jun 27, 2025

I apologize for the confusing comments and implementation.

I'll create a separate struct so that information other than port mapping can be written to network-config.json.

This change will be carried over to this PR!

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.

4 participants