refactor gce.RegenerateMigInstancesCache() to use Instance.List #6955
Conversation
k8s-ci-robot
added
kind/feature
|
Hi @damikag. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
| for _, err := range errors { | ||
| func (c *cachingMigInfoProvider) isMigInCreatingOrDeletingInstanceState(mig Mig) bool { | ||
| migRef := mig.GceRef() | ||
| state, found := c.cache.GetMigInstancesState(migRef) |
There was a problem hiding this comment.
I haven't thought about this before, but we need a better name for this. Maybe MigInstancesCount or MigInstancesStateCount? Because now, when I read below state[cloudprovider.InstanceCreating] it reads as state[state] and doesn't parse well in my head. It can be a separate PR though.
| func groupInstancesToMigs(instances []GceInstance) map[GceRef][]GceInstance { | ||
| migToInstances := map[GceRef][]GceInstance{} | ||
| for _, instance := range instances { | ||
| if _, found := migToInstances[instance.Igm]; found { |
There was a problem hiding this comment.
igm can be an empty GceRef, or a non-existent GceRef how do we want to handle that?
There was a problem hiding this comment.
We update cache only for the migs that are known and simply ignore instances from empty GceRefs or non-existent GceRefs. Did you notice any issue with that?
damikag
force-pushed
the
refactor-mig-fetch
branch
from
95c4ee6 to
22f6e7e
Compare
k8s-ci-robot
added
size/XL
damikag
force-pushed
the
refactor-mig-fetch
branch
2 times, most recently
from
201869c to
8ea6aec
Compare
| } | ||
| return updateErr | ||
| } | ||
|
|
||
| migs := c.migLister.GetMigs() | ||
| errors := make([]error, len(migs)) | ||
| workqueue.ParallelizeUntil(context.Background(), c.concurrentGceRefreshes, len(migs), func(piece int) { |
There was a problem hiding this comment.
Should this be skipped if bulkGceMigInstancesListingEnabled=true?
There was a problem hiding this comment.
Yes. We are doing the same thing in different ways.
…or listing MIG instances
damikag
force-pushed
the
refactor-mig-fetch
branch
from
8ea6aec to
8971a29
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bskiba, damikag, x13n The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
refactor gce.RegenerateMigInstancesCache() to use Instance.List API
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR refactor the RegenerateMigInstancesCache() to use Instance.List API when fetching MIG instances.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: