support variable sized pod-CIDRs for different-sized node (was: Allow --node-cidr-mask-size to change: make NodeIPAM controller support variable sized CIDRs)

[liuxu623]

• cluster cidr is 192.168.0.0/16
• set --node-cidr-mask-size 24
• add two nodes podCIDR will be 192.168.0.0/24 and 192.168.1.0/24
• set --node-cidr-mask-size 23 and restart controller-manager
• delete node 192.168.1.0/24
• add a node pod CIDR will be 192.168.0.0/23
• 192.168.0.0/24 and 192.168.0.0/23 overlap

I write some test code

func TestAllocate(t *testing.T) {
	_, clusterCIDR, _ := net.ParseCIDR("192.168.0.0/16")
	cs, _ := NewCIDRSet(clusterCIDR, 23)
	_, podCIDR, _ := net.ParseCIDR("192.168.0.0/24")
	cs.Occupy(podCIDR)
	_, podCIDR, _ = net.ParseCIDR("192.168.0.1/24")
	cs.Occupy(podCIDR)
	cs.Release(podCIDR)
	p,_:=cs.AllocateNext()
	fmt.Println(p.String())
}

[liuxu623]

/sig network

[andrewsykim]

It would be nice if node ipam can handle cases where either cluster-cidr or node-cidr-mask-size changes but generally I don't think changing them is supported.

[athenabot]

/triage unresolved

Comment /remove-triage unresolved when the issue is assessed and confirmed.

🤖 I am a bot run by vllry. 👩‍🔬

[aojea]

• cluster cidr is 192.168.0.0/16
• set --node-cidr-mask-size 24
• add two nodes podCIDR will be 192.168.0.0/24 and 192.168.1.0/24
• set --node-cidr-mask-size 23 and restart controller-manager
• delete node 192.168.1.0/24
• add a node pod CIDR will be 192.168.0.0/23
• 192.168.0.0/24 and 192.168.0.0/23 overlap

This has some consequences that I don't think Kubernetes can handle right now because it will imply that you have to delete all the nodes that overlap with the new mask.
In your example, if you don't delete one of the nodes or you reasign all the masks in all nodes, you can't increase the mask, otherwise, the new masks in one node will overlap with the masks of the other nodes.

[liuxu623]

• cluster cidr is 192.168.0.0/16
• set --node-cidr-mask-size 24
• add two nodes podCIDR will be 192.168.0.0/24 and 192.168.1.0/24
• set --node-cidr-mask-size 23 and restart controller-manager
• delete node 192.168.1.0/24
• add a node pod CIDR will be 192.168.0.0/23
• 192.168.0.0/24 and 192.168.0.0/23 overlap

This has some consequences that I don't think Kubernetes can handle right now because it will imply that you have to delete all the nodes that overlap with the new mask.
In your example, if you don't delete one of the nodes or you reasign all the masks in all nodes, you can't increase the mask, otherwise, the new masks in one node will overlap with the masks of the other nodes.

The problem is we use bitmap record the cidr is used or not now, but if we increase --node-cidr-mask-size, maybe more than one node use the cidr, so we need to know how many nodes use the cidr, if is zero we can use the cidr.

So I try use a map record how many nodes use the cidr, you can see #90926 and help me review the code, thank you.

[aojea]

The problem is we use bitmap record the cidr is used or not now, but if we increase --node-cidr-mask-size, maybe more than one node use the cidr, so we need to know how many nodes use the cidr, if is zero we can use the cidr.

ahhh ok, then you don't have to modify the bitmap, you just need to modify the Occupy() method to check if any of the bits are set between the begin and end returned by begin, end, err := s.getBeginingAndEndIndices(cidr)

	for i := begin; i <= end; i++ {
		if s.used.Bit(i) {
                     return fmt.Errorf("error some of the CIDRs are already allocated")
                 }
	}

kubernetes/pkg/controller/nodeipam/ipam/cidrset/cidr_set.go

Lines 224 to 239 in 7094849

	// Occupy marks the given CIDR range as used. Occupy does not check if the CIDR
	// range was previously used.
	func (s *CidrSet) Occupy(cidr *net.IPNet) (err error) {
	begin, end, err := s.getBeginingAndEndIndices(cidr)
	if err != nil {
	return err
	}
	s.Lock()
	defer s.Unlock()
	for i := begin; i <= end; i++ {
	s.used.SetBit(&s.used, i, 1)
	}
	return nil
	}

The comment on the method explicitly says Occupy does not check if the CIDR // range was previously used. but I don't know the context. maybe @bowei knows better.

I still have doubts that node-mask resizing can be supported, I think that the AllocateNext() method will not work after that 🤔 , or may cause conflicting assignments

[liuxu623]

I still have doubts that node-mask resizing can be supported, I think that the AllocateNext() method will not work after that 🤔 , or may cause conflicting assignments

The problem is node 192.168.0.0/24 and 192.168.1.0/24 all use 192.168.0.0/23，only 192.168.0.0/24 and 192.168.1.0/24 all deleted then we can release 192.68.0.0/23, but we use bitmap now, it can only record the cidr use or not, it can't record how many nodes use the cidr, delete a node 192.168.0.0/24 or 192.168.1.0/24 all will release 192.68.0.0/23.

// Release releases the given CIDR range.
func (s *CidrSet) Release(cidr *net.IPNet) error {
	begin, end, err := s.getBeginingAndEndIndices(cidr)
	if err != nil {
		return err
	}
	s.Lock()
	defer s.Unlock()
	for i := begin; i <= end; i++ {
		s.used.SetBit(&s.used, i, 0)
	}
	return nil
}

So I try use map to record how many nodes use the cidr, when delete a node s.used[i]--，AllocateNext() will not use the cidr if s.used[i] > 0, AllocateNext() will work normal.

// Release releases the given CIDR range.
func (s *CidrSet) Release(cidr *net.IPNet) error {
	begin, end, err := s.getBeginingAndEndIndices(cidr)
	if err != nil {
		return err
	}
	s.Lock()
	defer s.Unlock()
	for i := begin; i <= end; i++ {
		s.used.SetBit(&s.used, i, 0)
		if s.used[i] > 0 {
			s.used[i]--
		}
	}
	return nil
}

[liuxu623]

ahhh ok, then you don't have to modify the bitmap, you just need to modify the Occupy() method to check if any of the bits are set between the begin and end returned by begin, end, err := s.getBeginingAndEndIndices(cidr)

If Occupy() return error, NodeIPAMController will not start, we don't want this.

[aojea]

Thanks for the explanation @liuxu623 , it makes more sense to me now.
However, this seems an important change on the behavior of the IPAM and usually a KEP is required.
Let's ask Tim what will be the best way to proceed

/assign @thockin

[thockin]

The short answer is that this field was not designed to be changed.

The longer answer is that it COULD be (re)designed to be changeable, but as you point out that would require to change the bitmap-based allocator to something more sophisticated. AND it would have to be done in a way that we can upgrade existing clusters with existing allocations.

I won't say that we would not take such a change, but it would need to be KEP'ed and very carefully tested with regards to in-place upgrades.

[thockin]

I am going to re-title this and declare it a feature request with relatively low prio. If someone wants to step up to implement, we can talk.

[liuxu623]

kubernetes/enhancements#1811

[fl-max]

I think expanding a CIDR (ie. /24 to /23) block is much more tedious than shrinking a CIDR (ie. /24 to /25). Assuming this is true, maybe it would be better to split this into two features; "Expand CIDR mask size" and "Shrink CIDR mask size". I believe the latter would be much simpler to implement.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[pacoxu]

@aojea OK. Let me follow up.
/assign

So I need to move on to the KEP first and then the PR.
I think I need to use the new KEP template and continue the PR kubernetes/enhancements#1811 here.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

[aojea]

/remove-lifecycle stale
There is a KEP in progress kubernetes/enhancements#2593

[pacoxu]

/unassign
since another KEP is in process.

[aojea]

let's assign to the KEP owner then :)
/assign @rahulkjoshi

[aojea]

/assign @rahulkjoshi

[k8s-ci-robot]

@aojea: GitHub didn't allow me to assign the following users: rahulkjoshi.

Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @rahulkjoshi

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.

[bnu0]

I dont think the KEP covers what is being demonstrated here, which really seems like a bug (one which I hit today trying to change kube-controller-manager's default node podcidr prefix from a /24 to a /23).

As far as I can tell, this issue is not looking for a way to re-ip existing nodes. It is showing that if you change the kube-controller-manager argument from /24 to /23, then when a new node registers it can be errantly given a cidr that overlaps with existing nodes which already have the previous cidr size. This is very dangerous, for obvious reasons.

This happens without any attempt to re-ip anything, when a node with a /24 is deleted, it marks the whole subnet (now a /23) as available and allocates it regardless of the neighboring /24.

[aojea]

I dont think the KEP covers what is being demonstrated here, which really seems like a bug (one which I hit today trying to change kube-controller-manager's default node podcidr prefix from a /24 to a /23).

ah, right, sorry, I overlooked all the details.
The KEP allows to use different sizes but it keeps the limitation about not able to change the size of the assigned CIDR.
It is a known limitation, maybe should be better documented, but this field was close to be deprecated too #57130 🤷

[thockin]

I don't think we want to fix the existing allocator, but instead get the newer proposal working and it SHOULD (if I understand) cover this use case. E.g. you would add a new config with the same base and a different mask, and somehow that should be preferred (need to figure that out?)

…

On Sun, Aug 8, 2021 at 1:33 PM Antonio Ojea ***@***.***> wrote: I dont think the KEP covers what is being demonstrated here, which really seems like a bug (one which I hit today trying to change kube-controller-manager's default node podcidr prefix from a /24 to a /23). ah, right, sorry, I overlooked all the details. The KEP allows to use different sizes but it keeps the limitation about not able to change the size of the assigned CIDR. It is a known limitation, maybe should be better documented, but this field was close to be deprecated too #57130 <#57130> 🤷 — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#90922 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVFP5CKBUCAVHL6F75TT33S2TANCNFSM4M4TGRFA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

[rahulkjoshi]

Yes Tim is right. You would delete the old config -- the finalizer preserves the config until the last Node using that range goes away, but it becomes un-allocatable. Then you add your new config and restart all the nodes to use the new cidr size.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[rahulkjoshi]

/remove-lifecycle stale

The PR mentioned above should handle the use-case where the flag value gets changed. Code is targeted to be merged in 1.24

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue or PR with /reopen
• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

[k8s-ci-robot]

@k8s-triage-robot: Closing this issue.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue or PR with /reopen
• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.