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

Remove Knative #661

Merged
merged 16 commits into from
Jul 4, 2022
Merged

Remove Knative #661

merged 16 commits into from
Jul 4, 2022

Conversation

csegarragonz
Copy link
Collaborator

@csegarragonz csegarragonz commented Jun 22, 2022

In this PR we remove our integration with Knative. We were not using any of Knative's features, and it was making the deployment more complex, slow, and verbose.

In contrast, now we deploy Faasm on a bare k8s cluster. This simplifies the installation and deployment scripts. In summary, deploying/removing Faasm from an existing k8s cluster is a one-liner:

inv k8s.deploy --workers <num_workers> [--sgx]

inv k8s.delete

I think I have removed all dangling references to Knative in the org (but I am surely missing some). See related PRs: faasm/experiment-base#51, faasm/experiment-faabric#32, faasm/experiment-openmp#18

@csegarragonz csegarragonz self-assigned this Jun 22, 2022
@@ -11,7 +11,7 @@ metadata:
spec:
containers:
- name: minio-main
image: faasm/minio:0.8.12
image: faasm/minio:{{ faasm_version }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought that, given that we were already templating some of the contents, it was cleaner to also template the faasm version, instead of updating all deployment files every time we bump the code version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What other contents are we templating? I had thought I'd removed all templating from the repo as it makes it difficult to review changes, and generally adds more complexity. With this change we're now mixing SGX and non-SGX files again which I'd also quite like to avoid (just to keep the standard non-SGX setup as simple as possible).

@@ -1,8 +1,8 @@
from time import sleep
from datetime import datetime
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Very messy diff. You may check the raw file here.

@csegarragonz csegarragonz marked this pull request as ready for review June 22, 2022 15:48
Copy link
Collaborator

@Shillaker Shillaker left a comment

Choose a reason for hiding this comment

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

This looks good, other than the templating which I'm not sure about. Happy to discuss offline.

faasmcli/faasmcli/tasks/k8s.py Outdated Show resolved Hide resolved
@@ -11,7 +11,7 @@ metadata:
spec:
containers:
- name: minio-main
image: faasm/minio:0.8.12
image: faasm/minio:{{ faasm_version }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What other contents are we templating? I had thought I'd removed all templating from the repo as it makes it difficult to review changes, and generally adds more complexity. With this change we're now mixing SGX and non-SGX files again which I'd also quite like to avoid (just to keep the standard non-SGX setup as simple as possible).

{
"name": "AZ_ATTESTATION_PROVIDER_URL",
"value": "https://faasmattprov.eus2.attest.azure.net",
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we were going to template this stuff, it might be cleaner to keep this in a YAML file, but wrap it with an {% if sgx %} to switch it on or off.

@csegarragonz
Copy link
Collaborator Author

@Shillaker do you think we can merge this in?

Copy link
Collaborator

@Shillaker Shillaker left a comment

Choose a reason for hiding this comment

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

Yes, sorry for the delay. LGTM 👍

@csegarragonz csegarragonz merged commit f26b680 into main Jul 4, 2022
@csegarragonz csegarragonz deleted the remove-knative branch July 4, 2022 08:18
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.

None yet

2 participants