support to reload CNI configs real-time if defaultNetName is not assigned in initialization

[mars1024]

When we use ocicni in our CRI (pouch), we found that CNI configs can not be reloaded real-time because the first ASCIIbetically network config's name will be defaultNetName forever after initialization , a new CNI config with new name and high-sort-order will not be updated to defaultNetName unless we restarts CRI. I don't think it's a proper behavior we expect.

And I think what we expect is that if defaultNetName is not empty, a CNI config with that network name will be used as the default CNI network, and container network operations will fail until that network config with specific defaultNetName is present and valid. But if defaultNetName is empty, CNI config should be loaded real-time and defaultNetName should be determined by file sorting, just like kubernetes cniNetworkPlugin, this will avoid CRI restarts when we want to import a new CNI config.
I hope this pr will have some help for above.

[openshift-ci-robot]

Hi @mars1024. Thanks for your PR.

I'm waiting for a cri-o member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[mars1024]

@rajatchopra @dcbw @mrunalp PTAL, thx~

[zhuangqh]

I have a same demand with you. If the defaultNetName isn't specified, dynamically choose the newest one.

[mars1024]

I have a same demand with you. If the defaultNetName isn't specified, dynamically choose the newest one.

Yes, I also think most of CRIs will use ocicni by setting defaultNetName to empty string. And in this scenario, real-time reload is necessary.

[mrunalp]

The PR seems reasonable to me. @dcbw ptal.

[mrunalp]

@mccv1r0 ptal.

[dcbw]

I agree with the overall purpose of this PR. However, before that can be done there is a problem that needs to be solved. And that problem is caching any existing pod attachments on-disk so that if/when the default network is updated, the existing pods are deleted using their original network and not the new one that was just detected.

That's basically a pre-requisite for changing CNI configs while things are running.

That would look something like renaming the podLock structure to podInfo and keeping a copied version of the runtimeConfig and JSON config sent to the plugin on ADD so that it could be sent on DEL too. And unfortunately, that probably has to be cached on-disk somewhere so that if the process exists (intentionally like podman, or a long-lived CRI segfaults and restarts) that the correct info can be recalled from disk for DEL.

[mars1024]

I agree with the overall purpose of this PR. However, before that can be done there is a problem that needs to be solved. And that problem is caching any existing pod attachments on-disk so that if/when the default network is updated, the existing pods are deleted using their original network and not the new one that was just detected.

That's basically a pre-requisite for changing CNI configs while things are running.

That would look something like renaming the podLock structure to podInfo and keeping a copied version of the runtimeConfig and JSON config sent to the plugin on ADD so that it could be sent on DEL too. And unfortunately, that probably has to be cached on-disk somewhere so that if the process exists (intentionally like podman, or a long-lived CRI segfaults and restarts) that the correct info can be recalled from disk for DEL.

I agree with this, and I can help to store runtimeConfig and network JSON config persistently on disk in another PR to ensure the consistency between ADD and DEL.

But this PR focus on reloading CNI config real-time during initialization if defaultName is not set by CRI. For example, if we start CRI with a initial CNI config called net1, after that we generate a new CNI config called net2 by Calico daemonset, we expect CRI will know and change to the new config if it comes first in file sorting, this will help us to avoid restarting CRI.

In the current ocicni，even defaultNetworkName is set, your concerns above are still possible happening，because default cni network's content can change even if its name stays the same when default cni config file changes and a new config file is added, in this situation, the consistency between ADD and DEL has already been broken.

So I think we can focus on this PR firstly, after that, I can help to resolve the consistency problem between ADD and DEL. Do you agree with this? @dcbw

[zhuangqh]

I agree with the overall purpose of this PR. However, before that can be done there is a problem that needs to be solved. And that problem is caching any existing pod attachments on-disk so that if/when the default network is updated, the existing pods are deleted using their original network and not the new one that was just detected.
That's basically a pre-requisite for changing CNI configs while things are running.
That would look something like renaming the podLock structure to podInfo and keeping a copied version of the runtimeConfig and JSON config sent to the plugin on ADD so that it could be sent on DEL too. And unfortunately, that probably has to be cached on-disk somewhere so that if the process exists (intentionally like podman, or a long-lived CRI segfaults and restarts) that the correct info can be recalled from disk for DEL.

I agree with this, and I can help to store runtimeConfig and network JSON config persistently on disk in another PR to ensure the consistency between ADD and DEL.

But this PR focus on reloading CNI config real-time during initialization if defaultName is not set by CRI. For example, if we start CRI with a initial CNI config called net1, after that we generate a new CNI config called net2 by Calico daemonset, we expect CRI will know and change to the new config if it comes first in file sorting, this will help us to avoid restarting CRI.

In the current ocicni，even defaultNetworkName is set, your concerns above are still possible happening，because default cni network's content can change even if its name stays the same when default cni config file changes and a new config file is added, in this situation, the consistency between ADD and DEL has already been broken.

So I think we can focus on this PR firstly, after that, I can help to resolve the consistency problem between ADD and DEL. Do you agree with this? @dcbw

reasonable

[dcbw]

I agree with this, and I can help to store runtimeConfig and network JSON config persistently on disk in another PR to ensure the consistency between ADD and DEL.

But this PR focus on reloading CNI config real-time during initialization if defaultName is not set by CRI. For example, if we start CRI with a initial CNI config called net1, after that we generate a new CNI config called net2 by Calico daemonset, we expect CRI will know and change to the new config if it comes first in file sorting, this will help us to avoid restarting CRI.

@mars1024 I understand that this is the purpose of the PR, and I agree with the purpose :) But the case that is not covered here is if container A starts up after net1 but before net2, then net2 is created and container A no longer can be torn down correctly.

In the current ocicni，even defaultNetworkName is set, your concerns above are still possible happening，because default cni network's content can change even if its name stays the same when default cni config file changes and a new config file is added, in this situation, the consistency between ADD and DEL has already been broken.

Yes, but it is much less likely that the contents of the network config file will change completely, than it is an additional file is dropped on-disk from an independent component that may sort higher. Usually when daemonsets drop files on-disk, they want to sort higher than the existing files (or else they would not have been started).

libcni currently has some caching operations but needs to be extended to handle config caching, which could then be used here.

[mars1024]

I agree with this, and I can help to store runtimeConfig and network JSON config persistently on disk in another PR to ensure the consistency between ADD and DEL.
But this PR focus on reloading CNI config real-time during initialization if defaultName is not set by CRI. For example, if we start CRI with a initial CNI config called net1, after that we generate a new CNI config called net2 by Calico daemonset, we expect CRI will know and change to the new config if it comes first in file sorting, this will help us to avoid restarting CRI.

@mars1024 I understand that this is the purpose of the PR, and I agree with the purpose :) But the case that is not covered here is if container A starts up after net1 but before net2, then net2 is created and container A no longer can be torn down correctly.

I've got your point.

In the current ocicni，even defaultNetworkName is set, your concerns above are still possible happening，because default cni network's content can change even if its name stays the same when default cni config file changes and a new config file is added, in this situation, the consistency between ADD and DEL has already been broken.

Yes, but it is much less likely that the contents of the network config file will change completely, than it is an additional file is dropped on-disk from an independent component that may sort higher. Usually when daemonsets drop files on-disk, they want to sort higher than the existing files (or else they would not have been started).

libcni currently has some caching operations but needs to be extended to handle config caching, which could then be used here.

OK, I think I can help to resolve the config caching problem before approving this PR. Do you think we need a bigger discussion about config caching ?

And I have something to add that the cniNetworkPlugin for dockershim in kubernetes also have the same risk of default network changing because the sync goroutine, should I fix it? @dcbw

[dcbw]

@mars1024 something like containernetworking/cni#661 would do most of the work, leaving ocicni to simply retrieve the cached config/args/capabilityargs on DEL rather than having to cache that stuff internally itself.

[mars1024]

@mars1024 something like containernetworking/cni#661 would do most of the work, leaving ocicni to simply retrieve the cached config/args/capabilityargs on DEL rather than having to cache that stuff internally itself.

OK, seems a better way, I'll take a look at your PR ~

[mars1024]

@dcbw A simple question :), why do you let ocicni retrieve cached configs on DEL but not let libcni do this directly? If we retrieve cached configs in DEL of libcni, there maybe no code changes in ocicni.

[mars1024]

hi, @dcbw @mrunalp I think config caching in cni containernetworking/cni#661 may be hanging for a while, can we promote this firstly before merging that? Because we really meet a problem about reloading CNI config as I described above.
Ping me freely if you have some suggestions (like config caching in ocicni), thanks a lot!

[mars1024]

Friendly ping @dcbw @mrunalp , do you have more suggestions on this?

[dcbw]

Status update: upstream CNI config caching support is still moving forward

[mars1024]

Status update: upstream CNI config caching support is still moving forward

Thanks for your reply, we're working on a forked repo temporarily with vendor alias, after CNI config caching is merged, we will go back to this. Thanks again for your efforts in upstream.

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mars1024
To complete the pull request process, please assign sameo
You can assign the PR to them by writing /assign @sameo in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mars1024]

Updated to resolve conflict

[mars1024]

/assign @dcbw

[mars1024]

Since CNI config caching has been merged in upstream, we can promote this PR, either. @dcbw

[haircommander]

Why make this a variable? It's only used once below

[mars1024]

@haircommander Func syncNetworkConfig will be called when config files change, so isDefaultNetNameSpecified will be used not only once.

[haircommander]

@mars1024 yeah I get that, but if you change this line to:
isDefaultNetNameSpecified: defaultNetName != ""
you avoid the needless allocation of a local variable

[mars1024]

@haircommander DefaultNetName can be assigned through user init or first-time load, if it is assigned by user init, I don't think we want it is changed no matter how cni config files change, but if it is assigned by first-time load, I think it should reload real-time.

So we can't make sure defaultNetName != "" is equal to isDefaultNetNameSpecified==true, because defaultNetName is always non-empty by user init or first-time load, and we only want to make some changes when defaultNetName is assigned by first-time load.

[mars1024]

BTW, I'm waiting for #52 merged, then we can promote this PR. @haircommander @dcbw

[haircommander]

#52 seems to be a WIP, and I'd personally like to promote this. Why are you waiting on #52, and would you be willing to rebase and move this forward? @mars1024

[mars1024]

#52 seems to be a WIP, and I'd personally like to promote this. Why are you waiting on #52, and would you be willing to rebase and move this forward? @mars1024

@haircommander
Because reload CNI config real-time will cause problems in CNI DEL just as discussion above, so real-time reload should be merged after cni config caching is done.
But I can help to use cni config caching for CNI DEL in this PR either, I'll update this soon.

[mars1024]

Let's go to the new party, see #58 , @dcbw @mrunalp @haircommander , thanks ~

[dcbw]

obsoleted by #58