Skip to content
This repository has been archived by the owner on Nov 2, 2023. It is now read-only.

support calico #13

Merged
merged 1 commit into from
Jun 14, 2022
Merged

support calico #13

merged 1 commit into from
Jun 14, 2022

Conversation

luckymrwang
Copy link
Member

Fix the bug which not absolutely compatible with calico openyurtio/raven#38

@njucjc
Copy link
Member

njucjc commented Jun 6, 2022

Please deal with golint error in CI first.

@luckymrwang
Copy link
Member Author

Please deal with golint error in CI first.

The latest code has been commited. golint error has been dealed.

@njucjc njucjc requested review from DrmagicE and BSWANG June 7, 2022 07:18
@DrmagicE
Copy link
Member

DrmagicE commented Jun 8, 2022

Thanks for the enhancement. Quite busy these days, I will start to review this Friday.

k8s.io/client-go v0.19.2
k8s.io/klog/v2 v2.2.0
sigs.k8s.io/controller-runtime v0.7.2
k8s.io/api v0.23.2
Copy link
Member

Choose a reason for hiding this comment

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

The latest kubernetes version that OpenYurt currently supports is 1.22 and most of the OpenYurt components use v0.22.3 as kubernetes library version.

So how about downgrading the version to v0.22.3.

Copy link
Member Author

Choose a reason for hiding this comment

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

The latest code has been commited. k8s components have been downgraded to v0.22.3.

Copy link
Member

@njucjc njucjc Jun 14, 2022

Choose a reason for hiding this comment

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

github.com/projectcalico/api@v0.0.0-20220505235232-ce7a5122e146 requires k8s.io/api@v0.23.2.

repalce v0.22.3 to v0.23.2 will generate rest.HTTPClientFor in clientset (make generate) which is not supported in v0.22.3

@@ -0,0 +1,81 @@
// Copyright (c) 2019 Tigera, Inc. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Please add OpenYurt Copyright to the files in pkg/ravencontroller/apis/calico/v3/ directory.
For example:

// Copyright 2020 The OpenYurt Authors.
// Copyright (c) 2019 Tigera, Inc. All rights reserved.

Copy link
Member Author

Choose a reason for hiding this comment

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

The latest code has been commited. Copyright 2020 The OpenYurt Authors have been added.

if v.Spec.Node != node.Name || v.Spec.State != "confirmed" {
continue
}
podCIDR = v.Spec.CIDR
Copy link
Member

@DrmagicE DrmagicE Jun 12, 2022

Choose a reason for hiding this comment

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

It is possible for nodes in calico to have multiple cidr block, so there can be multiple blockaffinities referring the same node.
Currently, the Gateway CR cannot handle multiple podCIDR circumstances well(NodeInfo.Subnet is a string). I think we may add a subnet slice to Gateway CR to handle this, and the raven-agent also needs some adjustments.

@BSWANG @njucjc any thoughts on this?

Copy link
Member Author

Choose a reason for hiding this comment

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

@DrmagicE You are right. I have realized it too when adding calico cidr block. As you mentioned that raven-agent and gateway CR must be adjusted, so I just get one cidr block to adapt current case. When raven-agent supported multiple cidr block, I will adjust calico to adapt it again.

Copy link
Member

@njucjc njucjc Jun 13, 2022

Choose a reason for hiding this comment

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

how about changing NodeInfo.Subnet(string) to NodeInfo.Subnets(slice),this field is obtained by the controller and will not affect the way users use it.

Copy link
Member

Choose a reason for hiding this comment

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

@njucjc good idea, agree.

Copy link
Member

Choose a reason for hiding this comment

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

@luckymrwang Can you change it together in this PR?

@DrmagicE
Copy link
Member

lgtm

@DrmagicE
Copy link
Member

@luckymrwang Please fix the conflict.

@luckymrwang luckymrwang force-pushed the calico_supported branch 2 times, most recently from 95b3c44 to e601ecb Compare June 13, 2022 12:58
@luckymrwang
Copy link
Member Author

luckymrwang commented Jun 13, 2022

@luckymrwang Please fix the conflict.

@DrmagicE The latest code has been commited. Conflict has been fixed.

@DrmagicE
Copy link
Member

@luckymrwang Please remove the binary file raven-controller-manager

@luckymrwang
Copy link
Member Author

@luckymrwang Please remove the binary file raven-controller-manager

@DrmagicE The latest code has been commited.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants