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.

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

Jump to bottom

Replicaset integration in virtual kubelet provider #383

Merged
merged 1 commit into from
Dec 4, 2020
Merged

Replicaset integration in virtual kubelet provider #383

merged 1 commit into from
Dec 4, 2020

Conversation

mlavacca
Copy link
Member

Description

This PR heavily changes the Liqo provider of the virtual kubelet intending to implement the management of foreign replicasets instead of pods. The CRUD operations until now "pod-oriented" have to be migrated toward an approach "replica-set" oriented, by mapping the operations as follows:

  • CreatePod has to create a new remote replicaset
  • UpdatePod has to update a remote replicaset
  • DeletePod has to delete a remote replicaset

The replicasets have one replica each that aims at mapping home pods one by one with foreign pods. The mapping of each home pod with a foreign pod is implemented by a label in the replicaset spec podTemplateSpec field.

Address #380

@coveralls
Copy link

coveralls commented Nov 19, 2020 •
edited
Loading

Pull Request Test Coverage Report for Build 385707795

  • 220 of 508 (43.31%) changed or added relevant lines in 24 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-1.9%) to 46.155%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/virtualKubelet/apiReflection/reflectors/outgoing/apiTypes.go 0 1 0.0%
pkg/virtualKubelet/apiReflection/reflectors/outgoing/endpointSlices.go 2 3 66.67%
pkg/virtualKubelet/apiReflection/reflectors/outgoing/secrets.go 1 2 50.0%
pkg/virtualKubelet/forge/configmaps.go 0 2 0.0%
pkg/virtualKubelet/forge/endpointslices.go 0 2 0.0%
pkg/virtualKubelet/forge/replicaSets.go 27 29 93.1%
pkg/virtualKubelet/forge/services.go 0 2 0.0%
pkg/virtualKubelet/storage/cache.go 0 3 0.0%
pkg/virtualKubelet/provider/node.go 0 4 0.0%
pkg/virtualKubelet/storage/test/mockCacheManager.go 0 4 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/virtualKubelet/apiReflection/reflectors/outgoing/configmaps.go 1 0%
pkg/virtualKubelet/storage/indexers.go 1 44.74%
Totals Coverage Status
Change from base Build 383326690: -1.9%
Covered Lines: 6986
Relevant Lines: 15136
💛 - Coveralls

@mlavacca mlavacca mentioned this pull request Nov 19, 2020
Closed
4 tasks
@mlavacca mlavacca force-pushed the mlv/replicaset-provider branch 3 times, most recently from 87c4ae2 to 4404f0c Compare November 21, 2020 17:47
@mlavacca mlavacca force-pushed the mlv/replicaset-provider branch 2 times, most recently from 08166d6 to 7553864 Compare November 25, 2020 11:18
@mlavacca mlavacca force-pushed the mlv/replicaset-provider branch 11 times, most recently from e9a1b24 to 918e561 Compare November 27, 2020 18:42
@mlavacca
Copy link
Member Author

/rebase

@adamjensenbot
Copy link
Collaborator

Rebase status: success!

@mlavacca mlavacca force-pushed the mlv/replicaset-provider branch 2 times, most recently from 985ad02 to c28c6f6 Compare December 2, 2020 10:33
@mlavacca mlavacca requested review from aleoli, palexster and alacuku and removed request for alacuku December 2, 2020 11:49
aleoli
aleoli reviewed Dec 3, 2020
View reviewed changes
pkg/virtualKubelet/forge/pods.go
volumesOut = append(volumesOut, v)
}
// copy all volumes of type Secret except for the default token
if v.Secret != nil && !strings.Contains(v.Secret.SecretName, "default-token") {
Copy link
Member

Choose a reason for hiding this comment

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

Why are we not appending the default token secret? I read that was the same also before these change, but I don' remember why

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess this is the heritage from our missing support to service account secrets. I will remove that condition in a new PR and I will check the correctness of the mechanism, if you agree

Copy link
Member

Choose a reason for hiding this comment

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

Yes, fine for me

pkg/virtualKubelet/provider/pods.go Outdated
Comment on lines 51 to 52
podTranslated, err = serviceEnv.TranslateServiceEnvVariables(podTranslated, pod.Namespace, nattedNS, apiController.CacheManager())
_, err = p.foreignClient.AppsV1().ReplicaSets(foreignReplicaset.Namespace).Create(context.TODO(), foreignReplicaset, metav1.CreateOptions{})
Copy link
Member

Choose a reason for hiding this comment

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

Probably we are missing service environment translations

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch, we were going to miss them. I introduced them again. Anyway, I have some doubts about that function: what happens if, let's say one over ten services hasn't been replicated yet? The translated pod will have all the service environment variables but one?

Copy link
Member

@aleoli aleoli Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, good point, but I think that is the same for local deployment. If the pod comes before that a service is available it can't have it in env var but has to discover it with DNS

I think that in both the cases you have to be sure that the service was available a lot of time before the creation of the pod

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I suppose there is the same issue even in local deployments.

@mlavacca mlavacca force-pushed the mlv/replicaset-provider branch 3 times, most recently from 9f2d737 to ab6ffc7 Compare December 4, 2020 12:20
@mlavacca mlavacca force-pushed the mlv/replicaset-provider branch 3 times, most recently from 4373d2b to 49830b8 Compare December 4, 2020 12:31
@mlavacca mlavacca requested a review from aleoli December 4, 2020 13:40
@mlavacca mlavacca removed the discovery label Dec 4, 2020
@palexster
Copy link
Member

/rebase

@mlavacca @palexster
Replicaset integration in LiqoProvider
712d094 
This commit replaces the foreign CRUD operations on pods with operations
on replicasets. In addition, some tests have been created, and the
forge pkg for the object manipulation has been introduced.
@mlavacca mlavacca merged commit c49a6f9 into master Dec 4, 2020
@mlavacca mlavacca deleted the mlv/replicaset-provider branch December 4, 2020 19:15
@palexster palexster added this to the 0.2 milestone Dec 14, 2020
@alacuku alacuku added the kind/feature New feature or request label Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@palexster palexster palexster approved these changes

@aleoli aleoli aleoli approved these changes

@alacuku alacuku Awaiting requested review from alacuku

Assignees
No one assigned
Labels
kind/feature New feature or request size/XXL
Projects
None yet
Milestone
0.2
Development

Successfully merging this pull request may close these issues.
6 participants
@mlavacca @coveralls @adamjensenbot @palexster @aleoli @alacuku