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

feat(work-pool): add support for external prefect servers #64

Merged
merged 5 commits into from
Sep 6, 2024

Conversation

parkedwards
Copy link
Contributor

@parkedwards parkedwards commented Sep 5, 2024

resolves https://linear.app/prefect/issue/PLA-219/support-for-prefectworkpool-pointing-to-external-prefect-servers-and

this PR adds ~3 new fields:

  1. spec.server.remoteApiUrl => to be used if the apiKey is set
  2. spec.apiKey.value => raw, plaintext API Key
  3. spec.apiKey.valueFrom => reference-able secret (via k8s Secret) - uses the corev1.EnvVarSource struct as the type

if either .apiKey attribute is set, we'll use the .remoteApiUrl for PREFECT_API_URL

if either .apiKey attribute is set, we'll set a PREFECT_API_KEY env var

Settings []corev1.EnvVar `json:"settings,omitempty"`
}

type PrefectServerReference struct {
// Namespace is the namespace where the Prefect Server is running
// Namespace is the namespace where the in-cluster Prefect Server is running
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wonder if we make Name and Namespace optional now

Copy link
Contributor

Choose a reason for hiding this comment

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

seems like we could at least make Namespace optional, will think on it some more

Copy link
Contributor

@mitchnielsen mitchnielsen left a comment

Choose a reason for hiding this comment

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

Great stuff, the changes here look good to me.

I know the test cases cover what's added, but I wanted to run through it in action to see if anything stood out to me. I added my testing notes below. Nothing actionable here, except potentially the last item about what might be causing a Prefect Cloud target endpoint to give me a 401. Could definitely be a misunderstanding on my part as I'm still learning.

Click to expand testing notes
  1. make docker-build
  2. make install
  3. make deploy IMG=controller:latest
  4. Edit Deployment to imagePullPolicy: Never
  5. kustomize build config/samples > prefect.yaml
  6. Change workers to 1 since I'm on a small local cluster
  7. Create demo namespace
  8. kubectl apply -f prefect.yaml -n demo

Saw some errors:

"Operation cannot be fulfilled on prefectworkpools.prefect.io "sample-pool": the object has been modified; please apply your changes to the latest version and try again"}

I remember seeing these in the past, I think it has to do with how quickly the reconcile loop triggers and something with the cache. Can dig into that separately, not related to this change.

❓ Work pool failed to come up at first - maybe we should wait for the Prefect Server to be healthy? (name or service not known)

Changed the storage class name to match what Rancher Desktop uses, but got:

Forbidden: spec is immutable after creation except resources.requests and volumeAttributesClassName for bound claims

Deleted and reapplied my manifests to get around it.

❗Realized the default WorkPool manifest points to prefect-ephemral but here we're using sqlite (per the defaults in kustomization.yaml).

Made API token in dev and used kubectl create secret to add it to the demo namespace.

Configured the WorkPool to use the new values, got:

prefect.exceptions.PrefectHTTPStatusError: Client error '401 Unauthorized' for url 'https://app.prefect.dev/api/accounts//workspaces//work_pools/sample-pool'

I have a feeling this isn't related to the changes here, but I do wonder if we should be clear about what a real URL would look like here - at least for Prefect Cloud. (that said, would someone use the Operator to deploy a Work Pool if they're using Prefect Cloud?)

@parkedwards
Copy link
Contributor Author

thanks for such thorough testing steps and documenting them! you're right - i think we'll need to either (1) document what valid API urls look like (eg. with Cloud, they'll need to include the account/workspace IDs) or (2) expose those as optional values that we'll use to construct the API url for them

@parkedwards
Copy link
Contributor Author

that said, would someone use the Operator to deploy a Work Pool if they're using Prefect Cloud

I think they would. for some, I imagine this operator being an alternative to the prefect-helm charts that folks are already using to provision their worker(s) that run jobs against Cloud

@parkedwards
Copy link
Contributor Author

@mitchnielsen i just pushed up this change to explicitly expose an accountid and workspaceid - TBD on if we keep this (vs. just documenting and expecting the user to bring their fully formed URL based on the installation type)

d923982

i'll give it a more thorough test in the morning

Copy link
Collaborator

@chrisguidry chrisguidry left a comment

Choose a reason for hiding this comment

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

This is great!!

@chrisguidry chrisguidry added the feature New feature label Sep 6, 2024
@chrisguidry chrisguidry merged commit 77c6c6a into main Sep 6, 2024
3 checks passed
@chrisguidry chrisguidry deleted the feat/worker-external-or-cloud branch September 6, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants