Skip to content

[fix] : use single copy of instances cache #276

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.

Sign up for GitHub

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

Merged
rahulait merged 3 commits into from
Dec 18, 2024
Merged

[fix] : use single copy of instances cache #276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bc69f45
use single copy of instances
Dec 17, 2024
c5368d9
use single copy of instance cache for node_controller as well
Dec 17, 2024
dc0e24c
let http to https test run a bit longer as it sometimes takes a bit l…
Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions cloud/linode/cloud.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ type linodeCloud struct {
routes cloudprovider.Routes
}

var instanceCache *instances

func init() {
cloudprovider.RegisterCloudProvider(
ProviderName,
Expand Down Expand Up @@ -97,7 +99,8 @@ func newCloud() (cloudprovider.Interface, error) {
Options.VPCNames = Options.VPCName
}

routes, err := newRoutes(linodeClient)
instanceCache = newInstances(linodeClient)
routes, err := newRoutes(linodeClient, instanceCache)
if err != nil {
return nil, fmt.Errorf("routes client was not created successfully: %w", err)
}
Expand All @@ -123,7 +126,7 @@ func newCloud() (cloudprovider.Interface, error) {
// create struct that satisfies cloudprovider.Interface
lcloud := &linodeCloud{
Copy link
Contributor

@okokes-akamai okokes-akamai Dec 17, 2024

Choose a reason for hiding this comment

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

The Initialize method here calls newInstances yet again for the Node Controller - do we want that one to share this instances struct, too?

Copy link
Contributor Author

@rahulait rahulait Dec 17, 2024

Choose a reason for hiding this comment

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

Thank you, good catch. Updated the PR to address this as well.

client: linodeClient,
instances: newInstances(linodeClient),
instances: instanceCache,
loadbalancers: newLoadbalancers(linodeClient, region),
routes: routes,
}
Expand All @@ -139,7 +142,7 @@ func (c *linodeCloud) Initialize(clientBuilder cloudprovider.ControllerClientBui
serviceController := newServiceController(c.loadbalancers.(*loadbalancers), serviceInformer)
go serviceController.Run(stopCh)

nodeController := newNodeController(kubeclient, c.client, nodeInformer)
nodeController := newNodeController(kubeclient, c.client, nodeInformer, instanceCache)
go nodeController.Run(stopCh)
}

Expand Down
4 changes: 2 additions & 2 deletions cloud/linode/node_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ type nodeController struct {
queue workqueue.TypedDelayingInterface[any]
}

func newNodeController(kubeclient kubernetes.Interface, client client.Client, informer v1informers.NodeInformer) *nodeController {
func newNodeController(kubeclient kubernetes.Interface, client client.Client, informer v1informers.NodeInformer, instanceCache *instances) *nodeController {
timeout := defaultMetadataTTL
if raw, ok := os.LookupEnv("LINODE_METADATA_TTL"); ok {
if t, _ := strconv.Atoi(raw); t > 0 {
Expand All @@ -52,7 +52,7 @@ func newNodeController(kubeclient kubernetes.Interface, client client.Client, in

return &nodeController{
client: client,
instances: newInstances(client),
instances: instanceCache,
kubeclient: kubeclient,
informer: informer,
ttl: timeout,
Expand Down
4 changes: 2 additions & 2 deletions cloud/linode/route_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ type routes struct {
routeCache *routeCache
}

func newRoutes(client client.Client) (cloudprovider.Routes, error) {
func newRoutes(client client.Client, instanceCache *instances) (cloudprovider.Routes, error) {
timeout := 60
if raw, ok := os.LookupEnv("LINODE_ROUTES_CACHE_TTL_SECONDS"); ok {
if t, _ := strconv.Atoi(raw); t > 0 {
Expand All @@ -77,7 +77,7 @@ func newRoutes(client client.Client) (cloudprovider.Routes, error) {

return &routes{
client: client,
instances: newInstances(client),
instances: instanceCache,
routeCache: &routeCache{
routes: make(map[int][]linodego.VPCIP, 0),
ttl: time.Duration(timeout) * time.Second,
Expand Down
36 changes: 24 additions & 12 deletions cloud/linode/route_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ func TestListRoutes(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
client := mocks.NewMockClient(ctrl)
routeController, err := newRoutes(client)
instanceCache := newInstances(client)
routeController, err := newRoutes(client, instanceCache)
assert.NoError(t, err)

client.EXPECT().ListInstances(gomock.Any(), gomock.Any()).Times(1).Return([]linodego.Instance{}, nil)
Expand All @@ -56,7 +57,8 @@ func TestListRoutes(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
client := mocks.NewMockClient(ctrl)
routeController, err := newRoutes(client)
instanceCache := newInstances(client)
routeController, err := newRoutes(client, instanceCache)
assert.NoError(t, err)

client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{validInstance}, nil)
Expand All @@ -82,7 +84,8 @@ func TestListRoutes(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
client := mocks.NewMockClient(ctrl)
routeController, err := newRoutes(client)
instanceCache := newInstances(client)
routeController, err := newRoutes(client, instanceCache)
assert.NoError(t, err)

client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{validInstance}, nil)
Expand Down Expand Up @@ -123,7 +126,8 @@ func TestListRoutes(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
client := mocks.NewMockClient(ctrl)
routeController, err := newRoutes(client)
instanceCache := newInstances(client)
routeController, err := newRoutes(client, instanceCache)
assert.NoError(t, err)

client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{validInstance}, nil)
Expand Down Expand Up @@ -164,7 +168,8 @@ func TestListRoutes(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
client := mocks.NewMockClient(ctrl)
routeController, err := newRoutes(client)
instanceCache := newInstances(client)
routeController, err := newRoutes(client, instanceCache)
assert.NoError(t, err)

client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{validInstance}, nil)
Expand All @@ -179,7 +184,8 @@ func TestListRoutes(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
client := mocks.NewMockClient(ctrl)
routeController, err := newRoutes(client)
instanceCache := newInstances(client)
routeController, err := newRoutes(client, instanceCache)
assert.NoError(t, err)

vpcIP2 := "10.0.0.3"
Expand Down Expand Up @@ -283,7 +289,8 @@ func TestCreateRoute(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
client := mocks.NewMockClient(ctrl)
routeController, err := newRoutes(client)
instanceCache := newInstances(client)
routeController, err := newRoutes(client, instanceCache)
assert.NoError(t, err)

client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{validInstance}, nil)
Expand Down Expand Up @@ -315,7 +322,8 @@ func TestCreateRoute(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
client := mocks.NewMockClient(ctrl)
routeController, err := newRoutes(client)
instanceCache := newInstances(client)
routeController, err := newRoutes(client, instanceCache)
assert.NoError(t, err)

client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{validInstance}, nil)
Expand All @@ -328,7 +336,8 @@ func TestCreateRoute(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
client := mocks.NewMockClient(ctrl)
routeController, err := newRoutes(client)
instanceCache := newInstances(client)
routeController, err := newRoutes(client, instanceCache)
assert.NoError(t, err)

client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{}, nil)
Expand Down Expand Up @@ -370,7 +379,8 @@ func TestDeleteRoute(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
client := mocks.NewMockClient(ctrl)
routeController, err := newRoutes(client)
instanceCache := newInstances(client)
routeController, err := newRoutes(client, instanceCache)
assert.NoError(t, err)

client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{}, nil)
Expand Down Expand Up @@ -400,7 +410,8 @@ func TestDeleteRoute(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
client := mocks.NewMockClient(ctrl)
routeController, err := newRoutes(client)
instanceCache := newInstances(client)
routeController, err := newRoutes(client, instanceCache)
assert.NoError(t, err)

client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{validInstance}, nil)
Expand Down Expand Up @@ -431,7 +442,8 @@ func TestDeleteRoute(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
client := mocks.NewMockClient(ctrl)
routeController, err := newRoutes(client)
instanceCache := newInstances(client)
routeController, err := newRoutes(client, instanceCache)
assert.NoError(t, err)

client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{validInstance}, nil)
Expand Down
2 changes: 1 addition & 1 deletion e2e/test/lb-with-http-to-https/chainsaw-test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ spec:
port_443=$(curl --resolve linode.test:443:$IP --cacert ../certificates/ca.crt -s https://linode.test:443 | grep "test-")

if [[ -z $port_80 || -z $port_443 ]]; then
sleep 10
sleep 20
else
echo "all pods responded"
break
Expand Down
Loading