Integrate Azure Functions deployment with In-Vitro by cavinkavi · Pull Request #569 · vhive-serverless/invitro

cavinkavi · 2024-12-19T14:11:52Z

Summary

A small summary of the requirements (in one/two sentences).

To integrate Azure Functions deployment with In-Vitro. This feature enables deployment of serverless functions on the Azure Functions platform.

Implementation Notes ⚒️

Briefly outline the overall technical solution. If necessary, identify talking points where the reviewer's attention should be drawn to.

This PR adds a workflow to deploy multiple functions to Azure Functions via ZIP deployment.

Documentation on the steps to deploy the functions to Azure are included in loader.md.

azurefunctions_setup: this folder contains the relevant config and workload files to be used for deployment.
azure_functions.go: initializes Azure resources required for deployment to Azure Functions, such as Resource Group, Storage Account, and Function App. In order to deploy multiple functions via ZIP, each function must have its own specific folder containing required dependencies, before zipping and deploying all the function folders as a single package. After successful deployment, local temporary folders holding the functions and the zip package are cleaned up.
(Note: All cleanup, including local temporary folders, will be done after entire experiment is completed.)

External Dependencies 🍀

gopkg.in/yaml.v3 for reading Azure configuration files.
Azure CLI for managing Azure resources and deployment.

Breaking API Changes ⚠️

N/A

Simply specify none (N/A) if not applicable.

azurefunctions_setup/shared_azure_workload/azureworkload.py

pkg/driver/deployment/azure_functions.go

wanghanchengchn · 2025-01-15T12:41:41Z

Looks like there is an error in the checks.

Missing commit signatures: It seems the commits are missing Signed-off-by signatures. Please use git commit -s -m "Your commit message" when committing.
Commit history: Consider squashing your commits into a single commit to maintain a clean history.

You can find more details about these requirements in our contributing guide:
https://github.com/vhive-serverless/vHive/blob/main/docs/contributing_to_vhive.md

wanghanchengchn · 2025-01-16T05:10:11Z

Hi @leokondrashov, I've completed my initial review of this PR. @cavinkavi has made the corresponding changes, and the current version looks good to me. Do you have any additional feedback or concerns?

leokondrashov

First, there are no tests for the infrastructure, please add. Better even add them into CI tests.
Second, does Azure support Go deployments? We don't use Python that much and I'm not sure that our main Python implementation is well-behaved. It would be preferable to use Go instead.

leokondrashov · 2025-01-17T06:29:54Z

azurefunctions_setup/shared_azure_workload/azureworkload.py

+# Simulate the busySpin function
+def busy_spin(duration_ms: int) -> None:
+    end_time = perf_counter() + duration_ms / 1000  # Convert ms to seconds
+    while perf_counter() < end_time:
+        continue
+
+# Convert TraceFunctionExecution
+def trace_function_execution(start: float, time_left_milliseconds: int) -> str:
+    time_consumed_milliseconds = int((time.time() - start) * 1000)
+    if time_consumed_milliseconds < time_left_milliseconds:
+        time_left_milliseconds -= time_consumed_milliseconds
+        if time_left_milliseconds > 0:
+            busy_spin(time_left_milliseconds)
+
+    return f"OK - {hostname}"


Please reuse this parts from existing Python implementation (server/trace-func-py/trace_func.py). Don't reimplement because this creates duplicates in the code and incorrect performance comparisons.

leokondrashov · 2025-01-17T06:38:42Z

No need to throw out the Python deployment, though. It could be an option to deploy one of them during runtime. But the default should be Go to align with other deployments.

leokondrashov

Also, please change the message of your commit to something meaningful. Addressing my comments doesn't make any sense when you are looking at the history of commits.

leokondrashov · 2025-01-20T01:59:15Z

pkg/driver/deployment/azure_functions.go

+}
+
+// Helper function to copy files
+func copyFile(src, dst string) error {


Put this function in utils

leokondrashov · 2025-01-24T03:28:58Z

pkg/driver/deployment/azure_functions_test.go

Here you have the end-to-end test. Please add a test that would check the potential zip file for being operational. This wouldn't require the Azure credentials, so we can run it without any concerns about spending. This test would show when we have problems with our file copies in the archive.

Please also add them to the CI, so we can see the results right in the PRs.

At the same time, we can have an end-to-end test by just running the loader.go with the correct config file and checking for the output. There is no reason to add the script that duplicates the experiment pipeline. Better to repurpose it to test specific parts of the pipeline (deploying/cleaning functions, checking for the zip health).

In that case, would it be better if I remove the azure_functions_test.go file and create an End-to-End test under workflows, which will run the go command and check the output as you have mentioned?

Yes, similar to existing e2e tests. But still, add the module tests that I mentioned. For them, you would need the *_test.go files. And don't forget to add them into CI.

leokondrashov

And still, the commit name doesn't make sense. In this PR, you introduce the Azure Function deployment. Please put this as the first line of the commit message (and the only, I don't see the reason to put any other comments there apart from sign-offs, feel free to disagree).

leokondrashov · 2025-02-03T00:45:00Z

pkg/driver/deployment/azure_functions_test.go

Please add separate CI for that

leokondrashov · 2025-02-03T00:45:37Z

.github/workflows/e2e_azure.yaml

+      - name: Install Golang
+        uses: actions/setup-go@v5
+        with:
+          go-version: 1.21


Look at other CI workflows and use same version (AWS e2e is outdated as well but we need to rework whole that part)

leokondrashov · 2025-02-03T00:48:07Z

.github/workflows/e2e_azure.yaml

+      - name: Run Zip File Health Test
+        run: go test -v ./pkg/driver/deployment/azure_functions_test.go -run TestZipHealth
+
+      - name: Run Cleanup Test
+        run: go test -v ./pkg/driver/deployment/azure_functions_test.go -run TestCleanup


This should be separate CI, not combined with e2e.

Please also add the unit test for deployment logic. I would want to see an easy troubleshooting route for failures of e2e test. Unit tests should cover most of the steps separately so we would see which ones fail and work on that instead of going into logs of e2e.

leokondrashov · 2025-02-03T01:00:13Z

azurefunctions_setup/extract-workload.py

+def extract_execute_function(src_path):
+    # Extract the execute_function logic from trace_func.py.
+    with open(src_path, "r") as f:
+        content = f.read()
+
+    # Use regex to extract the execute_function definition and body
+    match = re.search(r"def execute_function\(.*?\):.*?(?=def |\Z)", content, re.DOTALL)
+    if not match:
+        raise ValueError("execute_function() not found in trace_func.py")
+
+    return match.group(0)
+
+def inject_function(func_code, workload_path):


I don't know how this slipped from the previous review.

Please don't look inside the file and parse it to extract the implementation during deployment. What I proposed is to pull this function into a separate file and reuse it from both places. This way, during deployment, you only need to copy the file and import. I don't trust parsing the files, regexp are not as robust as you would think.

leokondrashov · 2025-02-03T01:03:14Z

docs/loader.md

+2. Log in as a user (Note: This will open a browser window to select Azure account):
+    ```bash
+    az login
+    ```


This is not always possible. There are no browser available on cloudlab nodes. Can you provide pure CLI method of logging in?

docs/loader.md

leokondrashov · 2025-02-21T05:31:36Z

azurefunctions_setup/shared_azure_workload/exec_func.py

It is exactly the same as in the server/trace-func-py. Please leave only one of them.

And add the other one in .gitignore file so we won't see it added in git status after we do deployment. Also add the note in the main py file for Azure deployment near the execute_function call about the fact that you need to copy it for local development.

leokondrashov · 2025-02-21T05:34:48Z

docs/loader.md

+---
+Notes:
+
+- Service Principal must be created before running experiment, as some environments do not have browsers (e.g. CloudLab). Perform these steps in an environment that allows launching of browser and use the generated credentials. 


Suggested change

- Service Principal must be created before running experiment, as some environments do not have browsers (e.g. CloudLab). Perform these steps in an environment that allows launching of browser and use the generated credentials.

- Service Principal must be created before running the experiment, as some environments do not GUI (e.g., CloudLab nodes). You can perform these steps in an environment that allows the launching of the browser and use the generated credentials.

Overall, can this be done via Azure web console? This is much simpler than doing it via CLI, since this part you don't need to do in experiment environment.

leokondrashov · 2025-02-21T05:40:23Z

.github/workflows/unit-tests-azure.yaml

Instead of whole this file, you can add the directory into modules to test in unit-tests.yaml. Either way, you run all of the tests that you wrote in azure_functions_test.go but you specify all of them one-by-one, which is prone to errors (add test, forgot to test or the opposite, it fails because you removed one).

leokondrashov · 2025-02-21T05:46:44Z

pkg/driver/deployment/azure_functions_test.go

+func mockExecCommand(output string, err error) deployment.CommandRunner {
+	return func(name string, arg ...string) *exec.Cmd {
+		cmd := exec.Command("echo", output) // Simulate success
+		if err != nil {
+			return exec.Command("false") // Simulate failure
+		}
+		return cmd
+	}
+}


What does this thing do? I only see it used once with err=nil. It overloads the code with unnecessary flexibility.

leokondrashov · 2025-02-21T05:49:34Z

pkg/driver/deployment/azure_functions_test.go

+	assert.Contains(t, logs, "Deployed all 2 functions successfully")
+}
+
+// Tests CleanUpDeploymentFiles function correctly removes the specified temporary files and directories.


This test tests just two rm commands. It seems excessive for me. When I was talking about cleanup I meant the cleaning up the deployment in Azure.

leokondrashov · 2025-02-21T05:52:28Z

pkg/driver/deployment/azure_functions_test.go

+}
+
+// Tests DeployFunction function by simulating deployment of zip file to Azure.
+func TestDeployFunction(t *testing.T) {


This test just deploys the functions without cleaning them. So, my guess is that it fails on the second time onwards because functions already exist. Please test cleaning them up in the same test, so we can see whether it works. Also, be careful with the collision of tests. I.e., you shouldn't mistakenly remove the e2e functions that happen to run at the same time.

leokondrashov · 2025-02-21T05:54:26Z

pkg/driver/deployment/azure_functions.go

+
+func ZipFunctionAppFiles() error {
+	// Use bash to zip the contents of azure_functions_for_zip/ along with host.json and requirements.txt
+	cmd := exec.Command("bash", "-c", "cd azure_functions_for_zip && zip -r ../azurefunctions.zip . && cd .. && zip -j azurefunctions.zip azurefunctions_setup/host.json azurefunctions_setup/requirements.txt")


Please add all of the temporary directories and files into .gitignore

leokondrashov · 2025-02-21T05:58:56Z

pkg/driver/deployment/azure_functions.go

+
+	// Deploy the zip file to Azure Function App using CLI
+	cmd := runner("az", "functionapp", "deployment", "source", "config-zip",
+		"--name", config.AzureConfig.FunctionAppName,


There is an issue with deploying all functions into the same application. In Azure, the scaling unit is an application, so when you invoke one of the functions, it will instantiate all of the functions in the app. So, if you call other functions in the application, it would be a warm start instead of a cold start. We should model independent scaling for all functions, so they should be in different applications.

leokondrashov · 2025-02-21T06:03:11Z

pkg/driver/deployment/azure_functions.go

+
+/* Functions for deploying zipped functions */
+
+func DeployFunction(config *Config, function []*common.Function, runner CommandRunner) error {


BTW, this name is confusing, it deploys all the functions, not just one

Signed-off-by: Kavithran <104263022+cavinkavi@users.noreply.github.com>

wanghanchengchn self-requested a review January 9, 2025 08:36