Skip to content
This repository has been archived by the owner on Mar 9, 2022. It is now read-only.

Support replicaset and deployment info #342

Merged
merged 20 commits into from
Aug 22, 2018

Conversation

FredrikFolkesson
Copy link
Contributor

Supporting replicaset and deployment info.

Also changing sidecar container since the one we had before was very old and did not support newer APIs

Copy link
Contributor

@janmiderback janmiderback left a comment

Choose a reason for hiding this comment

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

Looks good! But, I have a few questions.

@@ -6,7 +6,7 @@ image:
pullPolicy: IfNotPresent
sideCartContainerName: kubectl
sideCartContainerArgs: "[ \"proxy\", \"-p\", \"8001\" ]"
sideCartContainerImage: gcr.io/google_containers/kubectl:v0.18.0-350-gfb3305edcf6c1a
sideCartContainerImage: lachlanevenson/k8s-kubectl:v1.9.10
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this source? Can we just trust it? Nothing more Google official?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The google one has not been updated since january 2017.

This is the source: https://github.com/lachie83/k8s-kubectl
It has most stars on github and most pulls on dockerhub from the images I found

logger.warn('Could not retrive deployment information, verify your RBAC settings');
}


Copy link
Contributor

Choose a reason for hiding this comment

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

Does not lint complain about this extra empty line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, but I removed it anyways ;)

@@ -54,8 +75,15 @@ class KubernetesClient {
const ip = pod.status.podIP;
const engine = { networks: [{ ip }], labels };
const key = pod.metadata.uid;
const replicaSet = replicaMap.get(pod.metadata.ownerReferences[0].uid);
Copy link
Contributor

Choose a reason for hiding this comment

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

So if it does not exist, does it become undefined?
There simply is no property in the response, or the key is is there but with value undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No key at all

* pod:
* type: object
* description: Pod information in verbatim format as returned by the Kubernetes API.
* replicaSet:
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we document that this key might not exist? Or, will it always exist but with possible value undefined? Should we clarify this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the key will not exist, but I will verify and add that info in the docummentation

Copy link
Contributor Author

@FredrikFolkesson FredrikFolkesson Aug 21, 2018

Choose a reason for hiding this comment

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

Verified that the key does not exist if the value is undefined.

The values are not marked as required in the swagger spec, so they are not expected to always exist.

Copy link
Member

@wennmo wennmo left a comment

Choose a reason for hiding this comment

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

LGTM

@coveralls
Copy link

coveralls commented Aug 21, 2018

Coverage Status

Coverage increased (+1.0%) to 88.315% when pulling 50ddb05 on support-replicaset-and-deployment-info into f937520 on master.

@janmiderback janmiderback requested a review from peol August 21, 2018 08:45
Copy link
Contributor

@janmiderback janmiderback left a comment

Choose a reason for hiding this comment

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

Some code coverage branches missed

replicaMap.set(item.metadata.uid, item);
});
} catch (error) {
logger.warn('Could not retrive replicaset information, verify your RBAC settings');
Copy link
Contributor

Choose a reason for hiding this comment

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

Add unit test that covers this case and verify expected output.

deploymentMap.set(item.metadata.uid, item);
});
} catch (error) {
logger.warn('Could not retrive deployment information, verify your RBAC settings');
Copy link
Contributor

Choose a reason for hiding this comment

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

Add unit test that covers this case and verify expected output.

@@ -40,6 +40,26 @@ class KubernetesClient {
* @returns {Promise<EngineContainerSpec[]>} A promise to a list of engine container specs.
*/
static async listEngines() {
const replicaMap = new Map();
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why we're doing these sequentially? AFAIK they don't use any data from previous calls and can be parallelized?

E.g.

const replicas = kubeHttpGet('/apis/apps/v1/replicasets');
const deployments = kubeHttpGet('/apis/apps/v1/deployments');
const result = await Promise.all([replicas, deployments]);
// result[0] = replicas, etc...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No you are right, we should definitely parallelize them :)

replicaMap.set(item.metadata.uid, item);
});
} catch (error) {
// Do nothing.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably log something if this fails, or?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried logging if it fails, since this will be the same on every "round" it gets very spammy.

Copy link
Contributor

@janmiderback janmiderback left a comment

Choose a reason for hiding this comment

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

Nice!

@FredrikFolkesson FredrikFolkesson merged commit c90d354 into master Aug 22, 2018
@FredrikFolkesson FredrikFolkesson deleted the support-replicaset-and-deployment-info branch August 22, 2018 10:51
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.

5 participants