fix: honor cacheready condition

[starbops]

IMPORTANT: Please do not create a Pull Request without creating an issue first.

Problem:

Solution:

Both ippool & vmnetcfg reconcile loops will honor ippool's cacheready condition, not depending on the internal cache testing. The agent will check the condition of the ippool object it syncs with, too

Related Issue:

harvester/harvester#5072

Test plan:

[Yu-Jack]

need to remove it?

[starbops]

updated, thanks!

[w13915984028]

Please continue to check the questions below:

(1) CacheReady is attached to CRD object networkv1.IPPool; but in fact, the cacheReady should be related to a local POD, not a remote CRD object.

The POD can restart at any-time, the POD itself needs a big lock/state to say, when it starts it's cache is not ready, it needs to sync unti the cache is build and ready, then all the controllers can continue to work based on it

The POD/Controller has an ipAllocator (as a shadow/view of remote IPPool CRD object, to simply the code process of allocating an IP), which is inited when the POD starts and works in the life-cycle of this POD. The IPPool controller and agent controller share it.

(2) Look below 3 functions, when a POD restarts, the networkv1.CacheReady is true because last time it was ready; the agent/ippool/ippool.go can still work before // ippool/controller.go OnChange set the CacheReady to false when POD starts.

// ippool/controller.go
// set !CacheReady
func (h *Handler) OnChange(key string, ipPool *IPPool) (*networkv1.IPPool, error) {
	if !h.ipAllocator.IsNetworkInitialized(ipPool.Spec.NetworkName) {
		networkv1.CacheReady.False(ipPoolCpy)
		networkv1.CacheReady.Reason(ipPoolCpy, "NotInitialized")
		networkv1.CacheReady.Message(ipPoolCpy, "")
		
// set CacheReady
func (h *Handler) BuildCache(ipPool *networkv1.IPPool, status networkv1.IPPoolStatus) (networkv1.IPPoolStatus, error) {
	logrus.Debugf("(ippool.BuildCache) build ipam for ippool %s/%s", ipPool.Namespace, ipPool.Name)

// agent/ippool/ippool.go 
func (c *Controller) Update(ipPool *networkv1.IPPool) error {
	if !networkv1.CacheReady.IsTrue(ipPool) {

[w13915984028]

please add ippool namespace and name into log, others are similar

[starbops]

Updated. PTAL, thanks

[w13915984028]

Please check harvester/harvester#4960 (comment)

The vm-dhcp-client has potential to be simplified to work only upon the existing VMNetworkConfig data, no necessary to build IPPool local cache, as the VMNetworkConfig is produced in control-plane.

[starbops]

Per your questions,

(1) Agents only sync with their IPPool objects; for example, the default-test-net-agent Pod syncs with the default/test-net IPPool. Agents don't rely on the controller's internal caches; they only honor the IPPool objects. It's one of the benefits of the control and data plane separation: if the controller dies, agents could still provide the service. The controller is responsible for maintaining the IPPool objects with the aid of the two internal caches. If the CacheReady condition is false, the controller cannot further reconcile the IPPool object. That means if new vmnetcfg objects are created, no IP addresses will be allocated for them. However, the agent can still serve their clients. It can rebuild the lease store from the ground up (based on the IPPool object) even if the agent Pod gets killed and respawned.

(2) In fact, I'm considering dropping the CacheReady condition check for the agent. As mentioned above, agents don't rely on the controller's internal caches, so checking that condition before syncing with the IPPool object makes little sense.

If there are any further questions, please let me know. Thank you.

[w13915984028]

The controller model is event-driven, it runs when each OnChange, OnRemove happens,

The retry.RetryOnConflict(retry.DefaultBackoff... will hold on a specific event's callback OnChage, as one OnChange plans to update two objects : vmnetworkconfig and ippool .

It is not the best solution, let's improve it later.

[w13915984028]

When conflict in RetryOnConflic, should re-get the ipPool from cache like above? current var ipPool may have been out-dated

ipPool, err := h.ippoolCache.Get(ipPoolNamespace, ipPoolName)

The original code is re-getting ipPool, what's the consideration to remove the code?

[starbops]

Yes, you're right. It should be inside the RetryOnConflict block. But I just realized we no longer need the explicit retry, as it was stale when internal caches were introduced.

[w13915984028]

LGTM, thanks.

[Yu-Jack]

LGTM

[starbops]

Merge conflict resolved.