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

(PXP-6977): make mds creds required for service startup #32

Merged

Conversation

johnfrancismccann
Copy link
Contributor

@johnfrancismccann johnfrancismccann commented Oct 19, 2020

Jira Ticket: PXP-6977

In combination with the indexing job updating the Metadata Service after file upload, require Indexd and MDS creds for all "name": "indexing" jobs configured for SSJDispatcher.

This SSJDispatcher PR should go out in the same monthly release as the corresponding indexing job PR.

Breaking Changes

  • Going forward, to support the indexing job updating the Metadata Service, SSJDispatcher now requires the Metadata Service to be deployed. Metadata Service creds are now required in the "imageConfig": { section for every "name": "indexing" job configured for SSJDispatcher. Without both Indexd and Metadata Service creds configured for all "name": "indexing" jobs, SSJDispatcher will exit immediately on startup.

Improvements

  • Exit with a status code of 1 instead of 0 when there are errors with reading the credentials file (e.g. it can't be found, no SQS url present, no MDS/Indexd creds)

Deployment changes

  • metadata-service must already be present or added to the "versions": { section of the cdis-manifest. After metadata-service has been confirmed to be in the cdis-manifest, the now-required Metadata Service creds for SSJDispatcher must be configured by running gen3 roll all with JENKINS_HOME not set. gen3 roll all calls gen3 kube-setup-ssjdispatcher, which now automatically configures the SSJDispatcher secret with Metadata Service creds.

Copy link

@frickjack frickjack left a comment

Choose a reason for hiding this comment

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

Awesome!

}

// Test that CheckIndexingJobsImageConfig returns an error when MDS creds
// have not been configured.
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't get why we're making this non backwards compatible - what if we need ssjdispatcher in a commons that doesn't use MDS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is meant as an extra precaution against the latest SSJDispatcher/indexs3client being deployed to a commons without MDS creds configured for SSJDispatcher, causing this new feature not to work. It's intended to be an automated way to signal to DevOps that SSJDispatcher/indexs3client are not healthy.

If SSJDispatcher is needed in a commons that doesn't use MDS, would it be reasonable to just use an older version of SSJDispatcher?

Also, one thing to keep in mind with this PR is that it only requires MDS creds for "name": "indexing" jobs, so it's not going to affect SSJDispatcher deployments in which there is no indexing job configured (not sure if that's the case for any commons).

Copy link
Contributor

@paulineribeyre paulineribeyre Oct 19, 2020

Choose a reason for hiding this comment

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

If SSJDispatcher is needed in a commons that doesn't use MDS, would it be reasonable to just use an older version of SSJDispatcher?

no, i don't think so. The ability to upload or index data (SSJDispatcher) is not dependent on whether the MDS is used. MDS is an optional service; all Commons should be able to get the latest SSJDispatcher patches regardless of whether they use MDS

Copy link
Contributor

Choose a reason for hiding this comment

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

we need to have the backward compatibility. Some commons do not require MDS

Choose a reason for hiding this comment

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

mds is no longer optional
there's no reason to not run mds
it adds complexity and new ways to eff up if we make it optional

Copy link
Contributor

Choose a reason for hiding this comment

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

then @johnfrancismccann can you update your "Deployment changes" to make it very clear that we need to add the MDS to the manifest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure, I'll make note of that in "Deployment changes" in the indexs3client PR as well.

Copy link
Contributor

@giangbui giangbui left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants