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.

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

calico fixes #7184

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

calico fixes #7184

wants to merge 1 commit into from

Conversation

disconn3ct
Copy link

Add more missing configs for Calico. With this config it joins the mesh successfully and seems to be working normally.

@MichaIng MichaIng added this to the v9.7 milestone Aug 14, 2024
@MichaIng MichaIng modified the milestones: v9.7, v9.8 Aug 26, 2024
@StephanStS StephanStS modified the milestones: v9.8, v9.9 Oct 27, 2024
@disconn3ct
Copy link
Author

What else should I do to get this merged?

@@ -44,6 +44,7 @@ CONFIG_BLK_DEV_NBD=m
CONFIG_BLK_DEV_NVME=y
CONFIG_BLK_DEV_RAM=n
CONFIG_BLK_DEV_SD=y
CONFIG_BLK_DEV_THROTTLING=y
Copy link
Owner

@MichaIng MichaIng Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That one seems to be pretty special, are you sure it is really used? E.g. the RPi kernel does not define it either. Probably it requires hardware support as well.

Comment on lines +348 to +350
CONFIG_IP_VS_FTP=n
CONFIG_IP_VS_PE_SIP=n
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you disable those explicitly? They are not defined (not even as "is not set") in our final kernel config, so it would not change something, but RPi and Debian kernels have the modules for those enabled. So, I guess either the lines can be removed, or we add the modules as well.

@@ -661,7 +673,7 @@ CONFIG_NFT_SOCKET=m
CONFIG_NFT_SYNPROXY=m
CONFIG_NFT_TPROXY=m
CONFIG_NFT_TUNNEL=m
CONFIG_NFT_XFRM=m
CONFIG_NFT_XFRM=n
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, this is set in RPi and Debian kernel. So disabling it explicitly, might beak something. If this module must not be loaded in your case, blacklist it via /etc/modprobe.d.

@@ -1122,3 +1134,16 @@ CONFIG_ZSWAP=y
CONFIG_ZSWAP_COMPRESSOR_DEFAULT_ZSTD=y
CONFIG_ZSWAP_DEFAULT_ON=y
CONFIG_ZSWAP_ZPOOL_DEFAULT_ZSMALLOC=y
CONFIG_RT_GROUP_SCHED=y
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as CONFIG_BLK_DEV_THROTTLING, this group scheduler is not available on RPi or Debian kernel either, so seems to be a very rare use case. Are you sure if is needed for Calico networks?

Comment on lines +1139 to +1141
CONFIG_INET_ESPINTCP=n
CONFIG_INET_ESP_OFFLOAD=n
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both are not defined in our kernel, so it shouldn't be required to explicitly disable them.

Comment on lines +1145 to +1149
CONFIG_XFRM_INTERFACE=n
CONFIG_XFRM_MIGRATE=n
CONFIG_XFRM_STATISTICS=n
CONFIG_XFRM_SUB_POLICY=n
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, why is it needed to explicitly disable those features? Or is it just that they get implicitly enabled with CONFIG_XFRM=y, and your aim was to enable only the minimal additional feature set?

@MichaIng
Copy link
Owner

Sorry for the long time it took to review it. I added some comments above. Most seems fine. I guess you explicitly disabled some features, which are implicitly enabled with their parent configs? IMO this is not needed. If we enable a parent feature, it is IMO fine to have the kernel enable additional sub features of it, according to its defaults. It might be confusing, if we tailor it too much, a way that users see a kernel module or feature, but not fully functional the way they are used to on plain Debian or other distros.

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.

3 participants