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

be explicit about ports for workers and servers #217

Conversation

technosophos
Copy link
Contributor

Closes #199

Signed-off-by: Matt Butcher [email protected]

Copy link
Member

@hongchaodeng hongchaodeng left a comment

Choose a reason for hiding this comment

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

lgtm

Signed-off-by: Matt Butcher <[email protected]>
@technosophos
Copy link
Contributor Author

Closes #182

| `gpu` | [`GPU`](#gpu) | N | | Specifies the attributes of the gpu resources required for the container. |
| `volumes` | [`[]Volume`](#volume) | N | | Specifies the attributes of the volumes that the container uses. |
| `extended` | [`[]ExtendedResource`](#extendedresource) | N | | Implementation-specific extended resource requirements |

For any resource that cannot be satisfied by the underlying platform, the platform MUST return an error and cease deployment. A resource is considered a requirement, and failure to meet that requirement means the runtime MUST NOT deploy the application. For example, if an application requests `1P` of memory, and that amount of memory is not available, the application deployment must fail. Likewise, if an application requires `1` gpu, and the runtime does not provide gpus, the application deployment MUST fail.

If resources are not specified, the underlying platform is free to choose a suitable default.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might cause weird & hard to debug errors. Is there a specific reason for making this to be not required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes -- issue #182

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And this was complicated by the fact that the way different platforms measure CPU and memory turn out to be different, as we discovered on Kubernetes a few weeks back.

Copy link
Contributor Author

@technosophos technosophos Oct 15, 2019

Choose a reason for hiding this comment

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

Or maybe I should ask. In Kubernetes, a sensible default for CPU is somewhere around 0.05 and maybe 0.1, because in Kubernetes, this is how much of a Kubernetes Node's actual CPU you need. I am assuming that for SF, the sane default is 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can do fractional cores as well, but I am thinking of cases where the default causes activation of the container to fail and makes it hard and not obvious for the operator to debug(we have seen such cases when we were doing SF Mesh).
Also, the issue #182 mentions Resources are optional for containers, they are mandatory as well in the current schema(is that comment meaning different resources than this one below?).
resources | Resources | Y |   | Resources required by the container.-- | -- | -- | -- | --

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed that section from this PR, opening another PR to deal with just #182

@technosophos technosophos force-pushed the fix/199-define-errors-for-server-worker branch from 42da44f to a788271 Compare October 15, 2019 21:24
Copy link
Contributor

@BharatNarasimman BharatNarasimman left a comment

Choose a reason for hiding this comment

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

:shipit:

@technosophos technosophos merged commit 95b4430 into oam-dev:master Oct 15, 2019
@technosophos technosophos deleted the fix/199-define-errors-for-server-worker branch October 15, 2019 21:27
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.

If a Server or SingletonServer does not have a port on the container, is this an error?
3 participants