fix(runJob/kubernetes): use explicit pod name #7039

ethanfrogers · 2019-05-21T15:17:53Z

the previous implementation of run job used manifest events to get the
name of the pod created by the job. this proved unreliable as events
roll off quickly in large environments. this commit implements a
PodNameProvider interface which supplies JobManifestPodLogs with the
name of the Pod to fetch logs from. Run Job supplies these Pod names by
collecting all Pods owned by the Job. For components which are still
event based, the JobEventBasedPodNameProvider implements the
previously used logic for getting the Pod name from the event.

ethanfrogers · 2019-05-21T15:19:27Z

@maggieneterval curious about how you feel about this approach. LMK!

There's a small bit of duplication but I think it's fine given that it's only used in 2 spots.

maggieneterval · 2019-05-21T21:21:36Z

Hey Ethan, approach seems sane to me! Would love to see a couple tests to help document some of the parsing logic, and maybe just throw your PR description as a comment on the interface sort of explaining why we implemented it?

ethanfrogers · 2019-05-22T13:13:33Z

@maggieneterval tests added! would you mind giving it another once over for an official review, please :)

maggieneterval

"official" (:laughing:) review added :heavy_check_mark:

And thanks for adding the tests!!

maggieneterval · 2019-05-22T15:29:49Z

app/scripts/modules/core/src/manifest/PodNameProvider.ts

+}
+
+export class DefaultPodNameProvider implements IPodNameProvider {
+  public podName: string;


maybe this can be private?

maggieneterval · 2019-05-22T15:30:56Z

app/scripts/modules/core/src/manifest/PodNameProvider.ts

+
+export class JobEventBasedPodNameProvider implements IPodNameProvider {
+  public manifest: IManifest;
+  public manifestEvent: IManifestEvent;


maybe these can be private as well?

maggieneterval · 2019-05-22T15:34:31Z

app/scripts/modules/core/src/manifest/stage/JobManifestPodLogs.tsx

-import { IManifestEvent, IManifest } from 'core/domain/IManifest';
-
-import { get, trim, bindAll } from 'lodash';
+import { bindAll } from 'lodash';


Ugh sorry I know you didn't create this file but can you please move the lodash import up to the third-part import block with his friends react etc.? 🙏

maggieneterval · 2019-05-22T15:35:16Z

app/scripts/modules/core/src/manifest/stage/JobStageExecutionLogs.tsx

@@ -5,15 +5,17 @@ import { IStageManifest, ManifestService } from '../ManifestService';
 import { JobManifestPodLogs } from './JobManifestPodLogs';
 import { IManifest } from 'core/domain/IManifest';

-import { get, template, isEmpty } from 'lodash';
+import { get, template, isEmpty, trim } from 'lodash';


Same here, lodash misses his BFF react

maggieneterval · 2019-05-22T15:36:24Z

...odules/core/src/pipeline/config/stages/preconfiguredJob/PreconfiguredJobExecutionDetails.tsx

@@ -8,9 +8,10 @@ import {
 } from 'core/pipeline';
 import { IPreconfiguredJobParameter } from './preconfiguredJobStage';
 import { JobStageExecutionLogs } from 'core/manifest/stage/JobStageExecutionLogs';
-import { get } from 'lodash';
+import { get, sortBy, last } from 'lodash';


Same here, react is v. lonely by himself in the first block

maggieneterval · 2019-05-22T15:37:19Z

...odules/core/src/pipeline/config/stages/preconfiguredJob/PreconfiguredJobExecutionDetails.tsx

@@ -61,9 +62,20 @@ export class TitusExecutionLogs extends React.Component<ITitusExecutionLogsProps
  }
 }

+interface IJobOwnedPodStatus {


I think we have this interface twice? Can we just export from one place?

the previous implementation of run job used manifest events to get the name of the pod created by the job. this proved unreliable as events roll off quickly in large environments. this commit implements a `PodNameProvider` interface which supplies `JobManifestPodLogs` with the name of the Pod to fetch logs from. Run Job supplies these Pod names by collecting all Pods owned by the Job. For components which are still event based, the `JobEventBasedPodNameProvider` implements the previously used logic for getting the Pod name from the event.

ethanfrogers · 2019-05-22T16:33:12Z

@spinnakerbot cherry-pick 1.14

spinnakerbot · 2019-05-22T16:35:55Z

Cherry pick successful: #7047

the previous implementation of run job used manifest events to get the name of the pod created by the job. this proved unreliable as events roll off quickly in large environments. this commit implements a `PodNameProvider` interface which supplies `JobManifestPodLogs` with the name of the Pod to fetch logs from. Run Job supplies these Pod names by collecting all Pods owned by the Job. For components which are still event based, the `JobEventBasedPodNameProvider` implements the previously used logic for getting the Pod name from the event.

a683832 fix(core): set runAsUser field correctly on triggers (#7048) f0287a1 fix(runJob/kubernetes): use explicit pod name (#7039) fc1cd1b fix(core): allow clearing of run as user field in triggers (#7045) 2466379 refactor(core): reactify CRON trigger (#7020)

the previous implementation of run job used manifest events to get the name of the pod created by the job. this proved unreliable as events roll off quickly in large environments. this commit implements a `PodNameProvider` interface which supplies `JobManifestPodLogs` with the name of the Pod to fetch logs from. Run Job supplies these Pod names by collecting all Pods owned by the Job. For components which are still event based, the `JobEventBasedPodNameProvider` implements the previously used logic for getting the Pod name from the event.

89e4e78 fix(runJob/kubernetes): reliably display logs (spinnaker#7060) f0287a1 fix(runJob/kubernetes): use explicit pod name (spinnaker#7039) 3fd183e fix(kubernetes): fix runjob stage init (spinnaker#7029) 4f4c32b feat(provider/kubernetes): namespace deployManifest (spinnaker#7016) 8e97d59 refactor(kubernetes): namespace selector component (spinnaker#7012) 53cb422 refactor(kubernetes): convert deploy manifest stage to react (spinnaker#7002) 0194821 feat(runjob): capture output ui (spinnaker#6978)

89e4e78 fix(runJob/kubernetes): reliably display logs (#7060) f0287a1 fix(runJob/kubernetes): use explicit pod name (#7039) 3fd183e fix(kubernetes): fix runjob stage init (#7029) 4f4c32b feat(provider/kubernetes): namespace deployManifest (#7016) 8e97d59 refactor(kubernetes): namespace selector component (#7012) 53cb422 refactor(kubernetes): convert deploy manifest stage to react (#7002) 0194821 feat(runjob): capture output ui (#6978)

ethanfrogers requested a review from maggieneterval May 21, 2019 15:18

ethanfrogers added the Do Not Merge Don't merge yet label May 21, 2019

ethanfrogers changed the title ~~fix(runJob/kubernetes): use explicit pod name~~ WIP fix(runJob/kubernetes): use explicit pod name May 21, 2019

ethanfrogers force-pushed the job-pod-names branch from 934da6f to 81e8051 Compare May 21, 2019 15:19

ethanfrogers force-pushed the job-pod-names branch from 81e8051 to 91b0fba Compare May 21, 2019 18:35

ethanfrogers force-pushed the job-pod-names branch 2 times, most recently from 2b6b690 to ea241e8 Compare May 22, 2019 13:12

ethanfrogers removed the Do Not Merge Don't merge yet label May 22, 2019

ethanfrogers changed the title ~~WIP fix(runJob/kubernetes): use explicit pod name~~ fix(runJob/kubernetes): use explicit pod name May 22, 2019

maggieneterval approved these changes May 22, 2019

View reviewed changes

ethanfrogers force-pushed the job-pod-names branch from ea241e8 to 4490842 Compare May 22, 2019 15:47

Merge branch 'master' into job-pod-names

e3b121d

ethanfrogers merged commit f0287a1 into spinnaker:master May 22, 2019

ethanfrogers deleted the job-pod-names branch May 22, 2019 16:32

spinnakerbot added the target-release/1.15 label May 22, 2019

anotherchrisberry mentioned this pull request May 22, 2019

chore(core): Bump version to 0.0.372 #7049

Merged

maggieneterval mentioned this pull request Jun 3, 2019

chore(kubernetes): Bump version to 0.0.23 #7073

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(runJob/kubernetes): use explicit pod name #7039

fix(runJob/kubernetes): use explicit pod name #7039

ethanfrogers commented May 21, 2019

ethanfrogers commented May 21, 2019 •

edited

maggieneterval commented May 21, 2019

ethanfrogers commented May 22, 2019

maggieneterval left a comment •

edited

maggieneterval May 22, 2019

maggieneterval May 22, 2019

maggieneterval May 22, 2019

maggieneterval May 22, 2019

ethanfrogers May 22, 2019

maggieneterval May 22, 2019

maggieneterval May 22, 2019

ethanfrogers commented May 22, 2019

spinnakerbot commented May 22, 2019

fix(runJob/kubernetes): use explicit pod name #7039

fix(runJob/kubernetes): use explicit pod name #7039

Conversation

ethanfrogers commented May 21, 2019

ethanfrogers commented May 21, 2019 • edited

maggieneterval commented May 21, 2019

ethanfrogers commented May 22, 2019

maggieneterval left a comment • edited

Choose a reason for hiding this comment

maggieneterval May 22, 2019

Choose a reason for hiding this comment

maggieneterval May 22, 2019

Choose a reason for hiding this comment

maggieneterval May 22, 2019

Choose a reason for hiding this comment

maggieneterval May 22, 2019

Choose a reason for hiding this comment

ethanfrogers May 22, 2019

Choose a reason for hiding this comment

maggieneterval May 22, 2019

Choose a reason for hiding this comment

maggieneterval May 22, 2019

Choose a reason for hiding this comment

ethanfrogers commented May 22, 2019

spinnakerbot commented May 22, 2019

ethanfrogers commented May 21, 2019 •

edited

maggieneterval left a comment •

edited