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

admin: tenant support jobs endpoints #84620

Merged

Conversation

dhartunian
Copy link
Collaborator

@dhartunian dhartunian commented Jul 18, 2022

Previously, the Job() and Jobs() endpoints were not implemented on the
tenant admin server as there was not a need and we had split the
implementation into a tenant-only server.

Now that we want to ship the jobs page into CC Console and support it
for multi-tenant, the endpoints for the UI should work as expected.

This PR edits the jobHelper and jobsHelper helpers to be functions
instead of methods in adminServer which allows the implementation to
be shared between the two servers.

Fixes #84621.

Release note: None

Release justification: low risk, high benefit addition to multi-tenant

@dhartunian dhartunian requested review from ericharmeling and a team July 18, 2022 22:18
@dhartunian dhartunian requested review from a team as code owners July 18, 2022 22:18
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

💯

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling)

Copy link
Contributor

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

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

A couple nits related to CI testing and local testing.

Reviewed all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian)


pkg/server/tenant_admin.go line 162 at r1 (raw file):

	}

	j, err := jobsHelper(

See below for context on this suggestion.

Suggestion:

	}

	t.sqlServer.cfg.TestingKnobs = base.TestingKnobs{}

	j, err := jobsHelper(

pkg/server/tenant_admin.go line 167 at r1 (raw file):

		userName,
		t.sqlServer,
		t.sqlServer.cfg,

When I test the jobs endpoint locally (using ./cockroach demo --multitenant=true), the cluster crashes with the following error:

* interface conversion: base.ModuleTestingKnobs is nil, not *jobs.TestingKnobs
* (1) attached stack trace
*   -- stack trace:
*   | runtime.gopanic
*   | 	GOROOT/src/runtime/panic.go:1038
*   | runtime.panicdottypeE
*   | 	GOROOT/src/runtime/iface.go:261
*   | runtime.panicdottypeI
*   | 	GOROOT/src/runtime/iface.go:271
*   | github.com/cockroachdb/cockroach/pkg/server.jobsHelper.func2
*   | 	github.com/cockroachdb/cockroach/pkg/server/admin.go:2124
*   | github.com/cockroachdb/cockroach/pkg/server.jobsHelper
*   | 	github.com/cockroachdb/cockroach/pkg/server/admin.go:2129
*   | github.com/cockroachdb/cockroach/pkg/server.(*tenantAdminServer).Jobs
*   | 	github.com/cockroachdb/cockroach/pkg/server/tenant_admin.go:162
* ...

Locally, I just set the problematic field to the zero value for base.TestingKnobs, and it resolved this issue. IDK if this is the best approach, but it worked for me. 😁

(Aside: I assume this was a problem because jobsHelper normally expects a pointer to server.cfg.BaseConfig, and not sqlServer.cfg and that field wasn't getting initialized properly?)


pkg/server/tenant_admin.go line 182 at r1 (raw file):
nit: linter is complaining

pkg/server/tenant_admin.go:176:1: receiver name s should be consistent with previous receiver name t for tenantAdminServer

Suggestion:

func (t *tenantAdminServer) Job(
	ctx context.Context, request *serverpb.JobRequest,
) (_ *serverpb.JobResponse, retErr error) {
	ctx = t.AnnotateCtx(ctx)

	userName, err := userFromContext(ctx)
	if err != nil {
		return nil, serverError(ctx, err)
	}
	r, err := jobHelper(ctx, request, userName, t.sqlServer)
	if err != nil {
		return nil, serverError(ctx, err)
	}
	return r, nil
}

Copy link
Collaborator Author

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

@ericharmeling PTAL, I added a separate commit that does some light refactoring of tests so they can be shared between packages.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @ericharmeling, and @knz)


pkg/server/tenant_admin.go line 167 at r1 (raw file):

Previously, ericharmeling (Eric Harmeling) wrote…

When I test the jobs endpoint locally (using ./cockroach demo --multitenant=true), the cluster crashes with the following error:

* interface conversion: base.ModuleTestingKnobs is nil, not *jobs.TestingKnobs
* (1) attached stack trace
*   -- stack trace:
*   | runtime.gopanic
*   | 	GOROOT/src/runtime/panic.go:1038
*   | runtime.panicdottypeE
*   | 	GOROOT/src/runtime/iface.go:261
*   | runtime.panicdottypeI
*   | 	GOROOT/src/runtime/iface.go:271
*   | github.com/cockroachdb/cockroach/pkg/server.jobsHelper.func2
*   | 	github.com/cockroachdb/cockroach/pkg/server/admin.go:2124
*   | github.com/cockroachdb/cockroach/pkg/server.jobsHelper
*   | 	github.com/cockroachdb/cockroach/pkg/server/admin.go:2129
*   | github.com/cockroachdb/cockroach/pkg/server.(*tenantAdminServer).Jobs
*   | 	github.com/cockroachdb/cockroach/pkg/server/tenant_admin.go:162
* ...

Locally, I just set the problematic field to the zero value for base.TestingKnobs, and it resolved this issue. IDK if this is the best approach, but it worked for me. 😁

(Aside: I assume this was a problem because jobsHelper normally expects a pointer to server.cfg.BaseConfig, and not sqlServer.cfg and that field wasn't getting initialized properly?)

what I just pushed up works without further tweaking of the config variables.


pkg/server/tenant_admin.go line 182 at r1 (raw file):

Previously, ericharmeling (Eric Harmeling) wrote…

nit: linter is complaining

pkg/server/tenant_admin.go:176:1: receiver name s should be consistent with previous receiver name t for tenantAdminServer

Done.

Copy link
Contributor

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

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

Nice! Apologies for the delay. There are still some linting errors, but otherwise LGTM. Thank you!

Reviewed 9 of 9 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian)

@dhartunian dhartunian force-pushed the sql-tenant-supports-jobs-endpoint branch 5 times, most recently from 1b09168 to 671f997 Compare August 30, 2022 15:15
Release note: None

Release justification: non-production code change
Previously, the Job() and Jobs() endpoints were not implemented on the
tenant admin server as there was not a need and we had split the
implementation into a tenant-only server.

Now that we want to ship the jobs page into CC Console and support it
for multi-tenant, the endpoints for the UI should work as expected.

This PR edits the `jobHelper` and `jobsHelper` helpers to be functions
instead of methods in `adminServer` which allows the implementation to
be shared between the two servers.

Release note: None

Release justification: low risk, high benefit addition to multi-tenant
@dhartunian dhartunian force-pushed the sql-tenant-supports-jobs-endpoint branch from 671f997 to 61d6af3 Compare August 30, 2022 17:08
@dhartunian
Copy link
Collaborator Author

TFTRs!

bors r=matthewtodd,knz,ericharmeling

@craig
Copy link
Contributor

craig bot commented Aug 30, 2022

Build succeeded:

@craig craig bot merged commit 1c96b93 into cockroachdb:master Aug 30, 2022
@blathers-crl
Copy link

blathers-crl bot commented Aug 30, 2022

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from e318993 to blathers/backport-release-22.1-84620: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@dhartunian dhartunian deleted the sql-tenant-supports-jobs-endpoint branch August 30, 2022 23:23
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.

tenant: support jobs endpoints on tenant HTTP server
5 participants