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.

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

GPII-3999: Apply the istio-proxy-manager solution to gpii-dataloader #442

Merged
merged 11 commits into from Aug 29, 2019

Conversation

@klown
Copy link
Contributor

commented Jul 8, 2019

@mrtyler @stepanstipl @amatas

This pull request implements the same istio-proxy-manager as the gpii-flushtokens cronjob, but here for the gpii-dataloader job. It is based on PR #478 and includes the changes from there.

I could have added it to the code in PR #478, but didn't want to disturb the status of that PR, namely, reviewed and ready to merge. This PR is ready for review.

NOTE: As of 09-Aug-2019, this pull request references a tagged version of the gpii-universal docker image:

repository: gpii/universal
tag: 20190809170605-f8485e9

NOTE2: Merging of this code depends on first merging the code in PR #478.

JIRA: https://issues.gpii.net/browse/GPII-3999

cc: @cindyli

**NOTE**: As of 09-Aug-2019, this pull request references @cindyli's development version of the docker image of gpii-universal: ``` repository: cindyqili/universal tag: latest ```

This MUST be replaced with the official tagged docker image provided by CI's tag calculation process (https://ci.gpii.net/job/docker-gpii-universal-master-calculate-tag/...).

**NOTE**: as of 01-Aug-2019, this pull request uses the same tagged version of the gpii-universal docker image used by #438, specifically:
repository: gpii/universal
tag: 20190801163411-26be63f

NOTE: as of 05-Jul-2019, this pull request depends on the tagged version of the gpii-universal docker image used by #438, specifically :

repository: gpii/universal
tag: 20190627203047-1262ac8
klown added 3 commits Jun 28, 2019
GPII-3999: Using the latest technique from flushtokens
Based on the latest yaml code for the gpii-flushtokens module, the
gpii-dataloader now completes the istio-proxy sidecar using the
3rd container approach.
@stepanstipl

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

Hi @klown, thanks, looks good to me 👍 (I've reviewed just the dataloader bit, but I assume we plan to merge this only after #383 just to make sure nothing else changed)

@klown

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

Thanks @stepanstipl. Works for me!

@amatas
amatas approved these changes Jul 9, 2019
Copy link
Contributor

left a comment

LGTM, perhaps we need to add a note to the description about the dependency of #383 to be merged first.

@klown

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

@amatas wrote:

LGTM, perhaps we need to add a note to the description about the dependency of #383 to be merged first.

Done.

@mrtyler
Copy link
Contributor

left a comment

LGTM with one question.

FYI Github is reporting a conflict with versions.yml, but I believe you have a plan for that.

done;
echo 'deleteAndLoadSnapsets.sh finished.';
echo 'Sending TERM to pilot-agent';
pkill pilot-agent;

This comment has been minimized.

Copy link
@mrtyler

mrtyler Jul 31, 2019

Contributor

Same question as here: #383 (comment)

This comment has been minimized.

Copy link
@klown

klown Aug 1, 2019

Author Contributor

Same response: added the loop.

klown added 2 commits Aug 1, 2019
GPII-3999: Addressing Tyler's review comment
Added loop to check that 'pilot-agent' process was terminated.
@klown

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

@mrtyler wrote:

FYI Github is reporting a conflict with versions.yml, but I believe you have a plan for that.

With the latest merge of master and flushtokens, the conflict has vanished.

@mrtyler

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

LGTM

@stepanstipl stepanstipl changed the base branch from master to GPII-2865-network-policies-yamls Aug 16, 2019

@stepanstipl stepanstipl changed the base branch from GPII-2865-network-policies-yamls to master Aug 16, 2019

@stepanstipl

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

@klown & others - I did change base of this PR to some random one & back, in order to force GH to refresh diff of this PR (as the master changed in the meantime). It now shows only the desired - small - diff of actual changes. And it looks good.

@klown

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

@stepanstipl as a followup to our GotoMeeting chat during today's architecture meeting, I've merged in the latest changes to master, and tested the istio-proxy-manager in my dev cluster. LGTM.

CC @mrtyler @amatas

stepanstipl added 2 commits Aug 29, 2019
@stepanstipl

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

I've just bumped the dataloader Helm chart version & I'm merging this.

@stepanstipl stepanstipl merged commit 058502a into gpii-ops:master Aug 29, 2019

4 checks passed

ci/circleci: exekube-versions-check Your tests passed on CircleCI!
Details
ci/circleci: gcp-unit-tests Your tests passed on CircleCI!
Details
ci/circleci: terraform-fmt-check Your tests passed on CircleCI!
Details
main Workflow: main
Details
@stepanstipl

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

done, deployed all the way to prod & all looks good, I’ve checked the logs and dataloader did run, did some work & then terminated nicely with istio. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.