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

Make CRD namespaced #95

Closed
ukclivecox opened this issue Feb 14, 2018 · 7 comments
Closed

Make CRD namespaced #95

ukclivecox opened this issue Feb 14, 2018 · 7 comments

Comments

@ukclivecox
Copy link
Contributor

Should the CRD definition be namespaced so it and the operator that uses it are all in one namespace instead of cluster-wide?

@Raab70
Copy link

Raab70 commented Feb 14, 2018

What's weird about this is that the CRD is scope Namespaced in the spec. I was able to get another seldon-core standing in a neighboring namespace by adding a flag to turn off the CRD definition and by fixing the hardcoded default namespaces for RBAC. After doing this I did confirm that the CRD is unavailable when trying to deploy a model to another namespace, but we're still getting this collision.

I also tried from there making the scope Cluster and deploying a model to another namespace. Which worked! But the pod was running in the default namespace...

This leads me to believe maybe theres some default namespaces hardcoded elsewhere that I missed?

@ukclivecox
Copy link
Contributor Author

That sounds like great progress. If the Deployment is created in the default namespace there must be something missing in the clusterManager java code for namespacing or how its being set.

Are you sure your SeldonDeployment has a namespace value?

@Raab70
Copy link

Raab70 commented Feb 14, 2018

It appears it does not...
Very strange, I can't seem to get it to set. I have tried:

  1. kubectl apply --namespace demomodel -f deployment.json
  2. setting up a context , kubectl config set-context demomodel --namespace=demomodel, as per https://kubernetes.io/docs/tasks/administer-cluster/namespaces-walkthrough/
  3. Specifying it in the deployment.json

EDIT: This looks like a Kubernetes problem. I was having this problem with 1.8.7 but upgrading to 1.9.2 appears to have fixed it.

If all goes well I'll open a PR.

@ukclivecox
Copy link
Contributor Author

I think there is an issue in the operator which is presently hardcoded to watch for new seldondeployments in the "default" namespace.

So I think there are changes required to

  • make the CRD namespaced
  • make the operator look for CRs created in its namespace

@Raab70
Copy link

Raab70 commented Feb 15, 2018

The CRD is namespaced but there is definitely a bug in deploying models to a namespace besides default. I was able to get seldon-core installed into the default namespace and another namespace as I mentioned above and I created two deployments:

  namespace: default
$ kubectl get sdep --namespace demomodel demo-model2-example -o yaml | grep namespace:
  namespace: demomodel

But the model never starts:

NAMESPACE     NAME                                                         READY     STATUS    RESTARTS   AGE
default       demo-model-deployment-demo-model-predictor-9c5dff6fd-grlb9   2/2       Running   0          2m
default       redis-df886d999-hdgwh                                        1/1       Running   0          7m
default       seldon-cluster-manager-75db85f5f9-bdxgg                      1/1       Running   0          7m
demomodel     redis-df886d999-dvckl                                        1/1       Running   0          2m
demomodel     seldon-cluster-manager-576c6ddf5-h9mj6                       1/1       Running   0          2m

I'll take a look at the operator and try to find where it's looking for CRs. Any guidance is appreciated.

Also, based on this I think we can close this issue, the real issue here is #89

@ukclivecox
Copy link
Contributor Author

Yep. We are working on a fix to make the CRD and the Operator fully namespaced.

@ukclivecox
Copy link
Contributor Author

Fixed by #97

  • The CRD is in a separate helm script which needs to be installed first and then you can install seldon-core into different namespaces.
  • The hardwired nodeports have been removed

See the example notebooks

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

No branches or pull requests

2 participants