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: Knative deployment support #206

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

octonawish-akcodes
Copy link
Collaborator

@octonawish-akcodes octonawish-akcodes commented Jun 20, 2024

Fully supported Knative serverless platform Integration PR cc @mcopik

Summary by CodeRabbit

  • New Features

    • Introduced cloud event handlers for Node.js and Python in Knative.
    • Added MinIO storage handling for Node.js and Python.
    • New configurations for Knative deployments in systems.json.
    • Added documentation for Knative deployment and configuration.
  • Bug Fixes

    • Ensured proper formatting in requirements.txt during package addition.
  • Chores

    • Automated Kind cluster setup with Knative via knative_setup.sh.
  • Documentation

    • Detailed instructions for setting up and deploying Knative in SeBS platform.

config/systems.json Outdated Show resolved Hide resolved
config/systems.json Outdated Show resolved Hide resolved
sebs/knative/config.py Show resolved Hide resolved
sebs/knative/config.py Show resolved Hide resolved
sebs/knative/config.py Show resolved Hide resolved
sebs/knative/storage.py Show resolved Hide resolved
sebs/knative/trigger.py Outdated Show resolved Hide resolved
sebs/knative/trigger.py Outdated Show resolved Hide resolved
sebs/knative/trigger.py Outdated Show resolved Hide resolved
sebs/knative/trigger.py Outdated Show resolved Hide resolved
@octonawish-akcodes octonawish-akcodes self-assigned this Jul 2, 2024
@octonawish-akcodes octonawish-akcodes marked this pull request as ready for review July 2, 2024 03:39
@octonawish-akcodes octonawish-akcodes changed the title [WIP] feat: Knative deployment support feat: Knative deployment support Jul 2, 2024
Signed-off-by: Abhishek Kumar <[email protected]>
Signed-off-by: Abhishek Kumar <[email protected]>
@octonawish-akcodes
Copy link
Collaborator Author

there are two things I want to discuss with you on wrappers and code packaging, Since the func deploy command automatically creates the function image, do we need to create the base image like we have for openwhisk, and func create creates files which includes several other files like req.txt, some bash script file, afaik and tested the main file is func.py and func.yaml so how should I package the code?

config/systems.json Outdated Show resolved Hide resolved
sebs/knative/function.py Outdated Show resolved Hide resolved
sebs/knative/knative.py Outdated Show resolved Hide resolved
sebs/knative/triggers.py Outdated Show resolved Hide resolved
Copy link

coderabbitai bot commented Jul 4, 2024

Note

Reviews paused

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Walkthrough

The update introduces comprehensive support for Knative within the SeBS platform, featuring new cloud event handlers, MinIO storage interactions, deployment configurations, and extensive testing capabilities. The changes span Node.js and Python environments, providing robust functionality for event handling, storage, and deployment automation.

Changes

File(s) Change Summary
benchmarks/wrappers/knative/nodejs/index.js, benchmarks/wrappers/knative/nodejs/storage.js Added cloud event handler and MinIO storage interaction class for Node.js.
benchmarks/wrappers/knative/python/func.py, benchmarks/wrappers/knative/python/storage.py Introduced main function for request handling and MinIO storage interaction class in Python.
config/systems.json Added Knative configurations for Python and Node.js environments.
docs/platforms.md Added deployment and configuration instructions for Knative within SeBS.
sebs/benchmark.py Enhanced package writing in add_deployment_package_python method by appending newline characters.
sebs/knative/config.py, sebs/knative/function.py, sebs/knative/knative.py, sebs/knative/triggers.py Introduced Knative system management classes and methods for configuration, function management, and triggers.
sebs/regression.py Added classes for regression testing specific to Knative for both Python and Node.js.
tools/knative_setup.sh Script to automate setup of a Kind cluster with Knative and Kourier, along with a local container registry.

In Knative's bright, new day,
Functions deploy in a seamless way.
With Node and Python, they rise,
Benchmarking under clear skies.
Storage flows with MinIO's grace,
All in one, it finds its place. 🌟


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 28

Outside diff range and nitpick comments (22)
sebs/knative/storage.py (3)

8-8: Consider adding a class docstring.

Adding a docstring to the KnativeMinio class can provide better context and understanding of its purpose and usage.

"""
KnativeMinio extends the Minio class to provide Knative-specific storage functionalities.
"""

13-19: Consider adding a docstring for the __init__ method.

Adding a docstring for the __init__ method can help clarify the purpose of each parameter.

"""
Initialize KnativeMinio.

Parameters:
- docker_client: Docker client instance.
- cache_client: Cache client instance.
- res: Resources instance.
- replace_existing: Boolean flag to indicate if existing storage should be replaced.
"""

22-28: Consider adding a docstring for the deserialize method.

Adding a docstring for the deserialize method can help clarify its functionality.

"""
Deserialize the KnativeMinio instance from cached configuration.

Parameters:
- cached_config: Cached Minio configuration.
- cache_client: Cache client instance.
- resources: Resources instance.

Returns:
- KnativeMinio instance.
"""
benchmarks/wrappers/knative/python/func.py (1)

8-42: Consider refining error handling and logging.

To provide more detailed error information and improve logging, you can include the stack trace in the log and return a more descriptive error message.

-        logging.error(f"Error - invocation failed! Reason: {e}")
+        logging.exception("Error - invocation failed!")

-        response = {
-            "begin": begin.strftime("%s.%f"),
-            "end": end.strftime("%s.%f"),
-            "results_time": results_time,
-            "result": f"Error - invocation failed! Reason: {e}",
-        }
+        response = jsonify({
+            "begin": begin.strftime("%s.%f"),
+            "end": end.strftime("%s.%f"),
+            "results_time": results_time,
+            "error": str(e),
+            "stack_trace": traceback.format_exc(),
+        })
sebs/knative/function.py (9)

11-33: Use X | Y for type annotations.

To follow modern Python type annotation practices, use X | Y instead of Optional[X].

-    storage: Optional[MinioConfig] = None
+    storage: MinioConfig | None = None
Tools
Ruff

15-15: Use X | Y for type annotations

Convert to X | Y

(UP007)


18-25: Consider adding a docstring for the deserialize method.

Adding a docstring for the deserialize method can help clarify its functionality.

"""
Deserialize the KnativeFunctionConfig instance from a dictionary.

Parameters:
- data: Dictionary containing the configuration data.

Returns:
- KnativeFunctionConfig instance.
"""

26-27: Consider adding a docstring for the serialize method.

Adding a docstring for the serialize method can help clarify its functionality.

"""
Serialize the KnativeFunctionConfig instance to a dictionary.

Returns:
- Dictionary containing the serialized data.
"""

29-33: Consider adding a docstring for the from_benchmark method.

Adding a docstring for the from_benchmark method can help clarify its functionality.

"""
Create a KnativeFunctionConfig instance from a benchmark.

Parameters:
- benchmark: Benchmark instance.

Returns:
- KnativeFunctionConfig instance.
"""

37-43: Consider adding a docstring for the __init__ method.

Adding a docstring for the __init__ method can help clarify the purpose of each parameter.

"""
Initialize KnativeFunction.

Parameters:
- name: Name of the function.
- benchmark: Benchmark associated with the function.
- code_package_hash: Hash of the code package.
- cfg: KnativeFunctionConfig instance.
"""

47-48: Consider adding a docstring for the config property.

Adding a docstring for the config property can help clarify its functionality.

"""
Get the KnativeFunctionConfig instance associated with this function.

Returns:
- KnativeFunctionConfig instance.
"""

50-52: Consider adding a docstring for the typename method.

Adding a docstring for the typename method can help clarify its functionality.

"""
Get the type name of this function.

Returns:
- String representing the type name.
"""

54-55: Consider adding a docstring for the serialize method.

Adding a docstring for the serialize method can help clarify its functionality.

"""
Serialize the KnativeFunction instance to a dictionary.

Returns:
- Dictionary containing the serialized data.
"""

57-78: Consider adding a docstring for the deserialize method.

Adding a docstring for the deserialize method can help clarify its functionality.

"""
Deserialize the KnativeFunction instance from a dictionary.

Parameters:
- cached_config: Dictionary containing the cached configuration data.

Returns:
- KnativeFunction instance.
"""
tools/knative_preparation.py (6)

4-9: Consider improving error handling and logging.

To provide more detailed error information and improve logging, you can include the stack trace in the log and return a more descriptive error message.

-        print(f"Error occurred: {e}")
+        print(f"Error occurred: {e}", file=sys.stderr)
+        traceback.print_exc()

14-20: Consider refining the logging messages.

To provide more detailed information, you can include the command being executed in the log messages.

-        print("Installing Minikube...")
+        print("Installing Minikube using curl...")

22-30: Consider refining the logging messages.

To provide more detailed information, you can include the command being executed in the log messages.

-        print("Installing kubectl...")
+        print("Installing kubectl using curl...")

32-39: Consider refining the logging messages.

To provide more detailed information, you can include the command being executed in the log messages.

-        print("Installing Cosign...")
+        print("Installing Cosign using curl...")

41-46: Consider refining the logging messages.

To provide more detailed information, you can include the command being executed in the log messages.

-        print("Installing jq...")
+        print("Installing jq using apt-get...")

48-73: Consider refining the logging messages.

To provide more detailed information, you can include the command being executed in the log messages.

-    print("Extracting images from the manifest and verifying signatures...")
+    print("Extracting images from the manifest and verifying signatures using curl...")

-    print("Installing Knative Serving component...")
+    print("Installing Knative Serving component using kubectl...")

-    print("Installing Knative Kourier networking layer...")
+    print("Installing Knative Kourier networking layer using kubectl...")

-    print("Fetching External IP address or CNAME...")
+    print("Fetching External IP address or CNAME using kubectl...")

-    print("Verifying Knative Serving installation...")
+    print("Verifying Knative Serving installation using kubectl...")
sebs/knative/triggers.py (3)

31-31: Clarify function name.

The function name get_command is confusing as it returns a list of arguments, not a command.

Consider renaming it to get_arguments for better clarity.


54-54: Use f-string instead of format call.

Convert the format call to an f-string for better readability.

- self.logging.error("Invocation of {} failed!".format(self.fname))
+ self.logging.error(f"Invocation of {self.fname} failed!")
Tools
Ruff

54-54: Use f-string instead of format call

Convert to f-string

(UP032)


80-84: Add type hints for constructor parameters.

Adding type hints improves code readability and helps with static analysis.

- def __init__(self, fname: str, url: str):
+ def __init__(self, fname: str, url: str) -> None:
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 0b8d777 and 5409e67.

Files selected for processing (11)
  • benchmarks/wrappers/knative/python/func.py (1 hunks)
  • config/systems.json (1 hunks)
  • sebs.py (1 hunks)
  • sebs/faas/config.py (1 hunks)
  • sebs/knative/config.py (1 hunks)
  • sebs/knative/function.py (1 hunks)
  • sebs/knative/knative.py (1 hunks)
  • sebs/knative/storage.py (1 hunks)
  • sebs/knative/triggers.py (1 hunks)
  • sebs/types.py (1 hunks)
  • tools/knative_preparation.py (1 hunks)
Files skipped from review due to trivial changes (1)
  • sebs/types.py
Additional context used
Ruff
sebs/knative/function.py

15-15: Use X | Y for type annotations

Convert to X | Y

(UP007)

sebs/knative/triggers.py

5-5: typing.Dict imported but unused

Remove unused import: typing.Dict

(F401)


40-45: Prefer capture_output over sending stdout and stderr to PIPE

Replace with capture_output keyword argument

(UP022)


54-54: Use f-string instead of format call

Convert to f-string

(UP032)

sebs/faas/config.py

213-213: Use config.get(name, config) instead of an if block

Replace with config.get(name, config)

(SIM401)

sebs/knative/config.py

2-2: sebs.faas.config.Credentials imported but unused

Remove unused import: sebs.faas.config.Credentials

(F401)

sebs/knative/knative.py

1-1: code imported but unused

Remove unused import: code

(F401)


2-2: os.devnull imported but unused

Remove unused import: os.devnull

(F401)


4-4: flask.config imported but unused

Remove unused import: flask.config

(F401)


6-6: sebs.faas.function.ExecutionResult imported but unused

Remove unused import: sebs.faas.function.ExecutionResult

(F401)


13-13: sebs.knative.triggers.KnativeLibraryTrigger imported but unused

Remove unused import: sebs.knative.triggers.KnativeLibraryTrigger

(F401)


14-14: sebs.faas.config.Resources imported but unused

Remove unused import: sebs.faas.config.Resources

(F401)


15-15: typing.Dict imported but unused

Remove unused import

(F401)


15-15: typing.Optional imported but unused

Remove unused import

(F401)


18-18: uuid imported but unused

Remove unused import: uuid

(F401)


30-30: Redefinition of unused config from line 4

(F811)


53-53: Redefinition of unused config from line 4

(F811)


149-154: Prefer capture_output over sending stdout and stderr to PIPE

Replace with capture_output keyword argument

(UP022)


158-158: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


222-222: Undefined name function_cfg

(F821)


232-237: Prefer capture_output over sending stdout and stderr to PIPE

Replace with capture_output keyword argument

(UP022)


240-245: Prefer capture_output over sending stdout and stderr to PIPE

Replace with capture_output keyword argument

(UP022)


255-260: Prefer capture_output over sending stdout and stderr to PIPE

Replace with capture_output keyword argument

(UP022)


286-286: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


292-292: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


305-319: Prefer capture_output over sending stdout and stderr to PIPE

Replace with capture_output keyword argument

(UP022)


326-326: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


333-333: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


352-352: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

Additional comments not posted (9)
tools/knative_preparation.py (1)

11-12: LGTM!

The function is_installed is well-structured and there are no issues.

sebs/knative/knative.py (7)

52-55: LGTM!

The config method correctly returns the Knative configuration.

Tools
Ruff

53-53: Redefinition of unused config from line 4

(F811)


57-59: LGTM!

The get_knative_func_cmd method correctly returns the Knative function command.


61-64: LGTM!

The function_type method correctly returns the specific function type for Knative.


71-86: LGTM!

The find_image method correctly checks if a Docker image exists in the repository.


166-200: Ensure the image tag includes knative.

The image tag should include knative to avoid mixing images between OpenWhisk and Knative.

Ensure that the image tag includes knative.


364-365: LGTM!

The name method correctly returns the name of the system.


27-37: Avoid redefinition of the config parameter.

The config parameter is redefined, which can cause confusion and potential issues.

- from flask import config

Likely invalid or redundant comment.

Tools
Ruff

30-30: Redefinition of unused config from line 4

(F811)

sebs.py (1)

91-91: LGTM!

The common_params

sebs/knative/triggers.py Outdated Show resolved Hide resolved
sebs/knative/triggers.py Outdated Show resolved Hide resolved
config/systems.json Outdated Show resolved Hide resolved
sebs/faas/config.py Show resolved Hide resolved
sebs/knative/config.py Outdated Show resolved Hide resolved
sebs/knative/knative.py Outdated Show resolved Hide resolved
sebs/knative/knative.py Outdated Show resolved Hide resolved
sebs/knative/knative.py Show resolved Hide resolved
sebs/knative/knative.py Outdated Show resolved Hide resolved
sebs/knative/knative.py Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 5409e67 and 4f1271e.

Files selected for processing (1)
  • benchmarks/wrappers/knative/python/storage.py (1 hunks)
Additional context used
Ruff
benchmarks/wrappers/knative/python/storage.py

3-3: json imported but unused

Remove unused import: json

(F401)

Additional comments not posted (7)
benchmarks/wrappers/knative/python/storage.py (7)

42-48: LGTM!

The unique_name method correctly generates a unique name for a file.


51-54: LGTM!

The upload method correctly handles file uploads and returns the unique key name.


56-57: LGTM!

The download method correctly handles file downloads.


59-63: LGTM!

The download_directory method correctly handles directory downloads.


65-70: LGTM!

The upload_stream method correctly handles stream uploads and returns the unique key name.


72-74: LGTM!

The download_stream method correctly handles stream downloads.


76-80: LGTM!

The get_instance method correctly implements the singleton pattern.

benchmarks/wrappers/knative/python/storage.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@oanarosca oanarosca left a comment

Choose a reason for hiding this comment

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

Two general comments:

  1. Please ensure the docs are fully updated to describe everything relevant about this (pretty extensive) change.
  2. Some comments from coderabbitai make sense to fix, e.g. unused imports and the like. Have a look through them.

I am left with one file to review - I'll get to it in a few hours' time.

config/systems.json Outdated Show resolved Hide resolved
benchmarks/wrappers/knative/python/func.py Show resolved Hide resolved
benchmarks/wrappers/knative/python/func.py Outdated Show resolved Hide resolved
benchmarks/wrappers/knative/python/func.py Outdated Show resolved Hide resolved
benchmarks/wrappers/knative/python/func.py Show resolved Hide resolved
tools/knative_preparation.py Outdated Show resolved Hide resolved
benchmarks/wrappers/knative/python/func.py Show resolved Hide resolved
sebs/knative/triggers.py Outdated Show resolved Hide resolved
sebs/knative/triggers.py Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 16

Outside diff range and nitpick comments (21)
benchmarks/wrappers/knative/python/func.py (2)

11-29: Suggestion: Move the timing code closer to the handler call for accuracy.

To ensure accurate timing, move the begin and end variables closer to the handler call.

-  begin = datetime.datetime.now()
-  ret = handler(event)
-  end = datetime.datetime.now()
+  ret = handler(event)
+  end = datetime.datetime.now()
+  begin = datetime.datetime.now()

31-41: Suggestion: Move the timing code closer to the handler call for accuracy.

To ensure accurate timing, move the begin and end variables closer to the handler call.

-  end = datetime.datetime.now()
-  results_time = (end - begin) / datetime.timedelta(microseconds=1)
+  results_time = (end - begin) / datetime.timedelta(microseconds=1)
+  end = datetime.datetime.now()
+  begin = datetime.datetime.now()
benchmarks/wrappers/knative/nodejs/index.js (2)

7-32: Suggestion: Move the timing code closer to the handler call for accuracy.

To ensure accurate timing, move the startTime and endTime variables closer to the handler call.

-  const startTime = new Date();
-  const result = await handler(eventData);
-  const endTime = new Date();
+  const result = await handler(eventData);
+  const endTime = new Date();
+  const startTime = new Date();

33-50: Suggestion: Move the timing code closer to the handler call for accuracy.

To ensure accurate timing, move the startTime and endTime variables closer to the handler call.

-  const endTime = new Date();
-  const resultTime = (endTime - startTime) / 1000; // Time in seconds
+  const resultTime = (endTime - startTime) / 1000; // Time in seconds
+  const endTime = new Date();
+  const startTime = new Date();
sebs/knative/function.py (3)

11-11: Use frozen=True for immutability.

Consider adding frozen=True to the @dataclass decorator to make instances of this class immutable.

- @dataclass
+ @dataclass(frozen=True)

30-33: Remove unnecessary super call.

The super call is unnecessary in this context and can be simplified.

-        return super(KnativeFunctionConfig, KnativeFunctionConfig)._from_benchmark(
-            benchmark, KnativeFunctionConfig
-        )
+        return KnativeFunctionConfig._from_benchmark(benchmark, KnativeFunctionConfig)

37-44: Consider using type hints for parameters.

Adding type hints improves readability and helps with static analysis.

-    def __init__(
-        self,
-        name: str,
-        benchmark: str,
-        code_package_hash: str,
-        cfg: KnativeFunctionConfig,
-    ):
+    def __init__(self, name: str, benchmark: str, code_package_hash: str, cfg: KnativeFunctionConfig):
tools/knative_preparation.py (1)

8-9: Use logging instead of print for error messages.

Using the logging module provides more flexibility and control over how messages are logged.

-        print(f"Error occurred: {e}")
+        logging.error(f"Error occurred: {e}")
sebs/knative/triggers.py (1)

10-14: Consider using type hints for parameters.

Adding type hints improves readability and helps with static analysis.

-    def __init__(self, fname: str, func_cmd: Optional[List[str]] = None

</blockquote></details>
<details>
<summary>config/systems.json (1)</summary><blockquote>

`237-277`: **Ensure completeness of Node.js deployment files.**

The `files` array for Node.js deployment is incomplete and contains empty strings.

```diff
- "files": [
-     "",
-     ""
- ]
+ "files": [
+     "index.js",
+     "storage.js"
+ ]

Ensure that the correct filenames are added.

sebs/knative/config.py (3)

1-5: Remove unused import.

The MinioConfig import is unused and should be removed.

- from sebs.storage.config import MinioConfig

53-53: Remove unused local variable.

The cached_config variable is assigned but never used.

- cached_config = cache.get_config("knative")

191-191: Remove unused local variable.

The cached_config variable is assigned but never used.

- cached_config = cache.get_config("knative")
Tools
Ruff

191-191: Local variable cached_config is assigned to but never used

Remove assignment to unused variable cached_config

(F841)

sebs/knative/knative.py (8)

13-13: Remove unused import.

The KnativeLibraryTrigger class is imported but not used anywhere in the file.

- from sebs.knative.triggers import KnativeLibraryTrigger, KnativeHTTPTrigger
+ from sebs.knative.triggers import KnativeHTTPTrigger
Tools
Ruff

13-13: sebs.knative.triggers.KnativeLibraryTrigger imported but unused

Remove unused import: sebs.knative.triggers.KnativeLibraryTrigger

(F401)


14-14: Remove unused imports typing.Dict and typing.Optional.

The Dict and Optional types are imported but not used anywhere in the file.

- from typing import Dict, Tuple, Type, List, Optional
+ from typing import Tuple, Type, List
Tools
Ruff

14-14: typing.Dict imported but unused

Remove unused import

(F401)


14-14: typing.Optional imported but unused

Remove unused import

(F401)


18-18: Remove redefined import.

The KnativeMinio import is redefined and should be removed.

- from sebs.knative.storage import KnativeMinio
Tools
Ruff

18-18: Redefinition of unused KnativeMinio from line 12

Remove definition: KnativeMinio

(F811)


206-206: Remove unused local variable.

The package_config variable is assigned but never used.

- package_config = CONFIG_FILES[language_name]
Tools
Ruff

206-206: Local variable package_config is assigned to but never used

Remove assignment to unused variable package_config

(F841)


263-263: Remove unused local variable.

The language variable is assigned but never used.

- language = code_package.language_name
Tools
Ruff

263-263: Local variable language is assigned to but never used

Remove assignment to unused variable language

(F841)


316-316: Raise exceptions with raise ... from err or raise ... from None.

Exceptions should be raised with raise ... from err or raise ... from None to distinguish them from errors in exception handling.

- raise RuntimeError("Failed to access func binary")
+ raise RuntimeError("Failed to access func binary") from None
Tools
Ruff

316-316: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


356-356: Raise exceptions with raise ... from err or raise ... from None.

Exceptions should be raised with raise ... from err or raise ... from None to distinguish them from errors in exception handling.

- raise RuntimeError(e)
+ raise RuntimeError(e) from e
Tools
Ruff

356-356: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


374-374: Raise exceptions with raise ... from err or raise ... from None.

Exceptions should be raised with raise ... from err or raise ... from None to distinguish them from errors in exception handling.

- raise RuntimeError(e)
+ raise RuntimeError(e) from e
Tools
Ruff

374-374: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 4f1271e and 8779f50.

Files selected for processing (12)
  • benchmarks/wrappers/knative/nodejs/index.js (1 hunks)
  • benchmarks/wrappers/knative/nodejs/storage.js (1 hunks)
  • benchmarks/wrappers/knative/python/func.py (1 hunks)
  • benchmarks/wrappers/knative/python/storage.py (1 hunks)
  • config/systems.json (1 hunks)
  • sebs/knative/init.py (1 hunks)
  • sebs/knative/config.py (1 hunks)
  • sebs/knative/function.py (1 hunks)
  • sebs/knative/knative.py (1 hunks)
  • sebs/knative/storage.py (1 hunks)
  • sebs/knative/triggers.py (1 hunks)
  • tools/knative_preparation.py (1 hunks)
Files skipped from review due to trivial changes (1)
  • sebs/knative/init.py
Additional context used
Biome
benchmarks/wrappers/knative/nodejs/storage.js

[error] 55-55: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 56-56: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 58-58: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

Ruff
benchmarks/wrappers/knative/python/storage.py

3-3: json imported but unused

Remove unused import: json

(F401)

sebs/knative/config.py

191-191: Local variable cached_config is assigned to but never used

Remove assignment to unused variable cached_config

(F841)

sebs/knative/knative.py

13-13: sebs.knative.triggers.KnativeLibraryTrigger imported but unused

Remove unused import: sebs.knative.triggers.KnativeLibraryTrigger

(F401)


14-14: typing.Dict imported but unused

Remove unused import

(F401)


14-14: typing.Optional imported but unused

Remove unused import

(F401)


18-18: Redefinition of unused KnativeMinio from line 12

Remove definition: KnativeMinio

(F811)


206-206: Local variable package_config is assigned to but never used

Remove assignment to unused variable package_config

(F841)


256-256: Undefined name function_cfg

(F821)


263-263: Local variable language is assigned to but never used

Remove assignment to unused variable language

(F841)


316-316: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


356-356: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


374-374: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

Additional comments not posted (19)
sebs/knative/storage.py (3)

10-11: LGTM!

The deployment_name method is simple and correct.


13-20: LGTM!

The __init__ method correctly initializes the KnativeMinio class.


23-28: LGTM!

The deserialize method is correctly implemented and follows the pattern of calling the parent class's method.

benchmarks/wrappers/knative/python/func.py (3)

1-5: LGTM!

The import statements are correct and necessary for the function implementation.


9-9: LGTM!

Setting the logging level at the beginning of the function is a good practice.


29-29: LGTM!

The return statements are correctly implemented.

Also applies to: 41-41

benchmarks/wrappers/knative/nodejs/index.js (3)

1-2: LGTM!

The import statements are correct and necessary for the function implementation.


10-10: LGTM!

Setting up logging configuration within the function is a good practice.

Also applies to: 16-16, 37-37


28-32: LGTM!

The return statements are correctly implemented.

Also applies to: 46-50

benchmarks/wrappers/knative/nodejs/storage.js (7)

11-25: LGTM!

The constructor is correctly implemented.


27-31: LGTM!

The unique_name method is correctly implemented.


33-36: LGTM!

The upload method is correctly implemented.


38-40: LGTM!

The download method is correctly implemented.


42-47: LGTM!

The uploadStream method is correctly implemented.


49-51: LGTM!

The downloadStream method is correctly implemented.


63-63: LGTM!

The export statement is correctly implemented.

benchmarks/wrappers/knative/python/storage.py (2)

3-3: Remove unused import.

The json module is imported but not used in the file.

- import json
Tools
Ruff

3-3: json imported but unused

Remove unused import: json

(F401)


38-39: Improve exception handling.

The current exception handling logs the exception and re-raises it. Consider logging the exception as an error instead of info for better visibility.

- logging.info(e)
+ logging.error(e)
tools/knative_preparation.py (1)

11-12: LGTM!

The function is_installed is straightforward and does not require changes.

sebs/knative/config.py Show resolved Hide resolved
benchmarks/wrappers/knative/nodejs/storage.js Outdated Show resolved Hide resolved
sebs/knative/function.py Show resolved Hide resolved
sebs/knative/function.py Outdated Show resolved Hide resolved
sebs/knative/function.py Show resolved Hide resolved
benchmarks/wrappers/knative/python/storage.py Show resolved Hide resolved
tools/knative_preparation.py Outdated Show resolved Hide resolved
tools/knative_preparation.py Outdated Show resolved Hide resolved
tools/knative_preparation.py Outdated Show resolved Hide resolved
sebs/knative/knative.py Outdated Show resolved Hide resolved
Signed-off-by: Abhishek Kumar <[email protected]>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 8779f50 and 088ca51.

Files selected for processing (1)
  • sebs/knative/knative.py (1 hunks)
Additional context used
Ruff
sebs/knative/knative.py

13-13: sebs.knative.triggers.KnativeLibraryTrigger imported but unused

Remove unused import: sebs.knative.triggers.KnativeLibraryTrigger

(F401)


14-14: typing.Dict imported but unused

Remove unused import

(F401)


14-14: typing.Optional imported but unused

Remove unused import

(F401)


18-18: Redefinition of unused KnativeMinio from line 12

Remove definition: KnativeMinio

(F811)


206-206: Local variable package_config is assigned to but never used

Remove assignment to unused variable package_config

(F841)


256-256: Undefined name function_cfg

(F821)


263-263: Local variable language is assigned to but never used

Remove assignment to unused variable language

(F841)


316-316: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


356-356: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


374-374: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

Additional comments not posted (6)
sebs/knative/knative.py (6)

56-58: LGTM!

The method constructs a command list for Knative functions correctly.


65-80: LGTM!

The method retrieves or initializes storage for Knative correctly.


82-97: LGTM!

The method checks if a Docker image exists in the repository correctly.


385-387: LGTM!

The method reinitializes logging handlers for all triggers correctly.


358-381: Use capture_output instead of sending stdout and stderr to PIPE.

The capture_output argument should be used for better readability and maintainability.

-                response = subprocess.run(
-                    ["func", "describe", function.name, "--output", "url"],
-                    stdout=subprocess.PIPE,
-                    stderr=subprocess.DEVNULL,
-                    check=True,
-                )
+                response = subprocess.run(
+                    ["func", "describe", function.name, "--output", "url"],
+                    capture_output=True,
+                    check=True,
+                )

Likely invalid or redundant comment.

Tools
Ruff

374-374: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


318-357: Use capture_output instead of sending stdout and stderr to PIPE.

The capture_output argument should be used for better readability and maintainability.

-            subprocess.run(
-                [
-                    "func",
-                    "deploy",
-                    "--path",
-                    code_package.code_location,
-                    "--image",
-                    docker_image,
-                    "--name",
-                    function.name,
-                ],
-                stderr=subprocess.PIPE,
-                stdout=subprocess.PIPE,
-                check=True,
-            )
+            subprocess.run(
+                [
+                    "func",
+                    "deploy",
+                    "--path",
+                    code_package.code_location,
+                    "--image",
+                    docker_image,
+                    "--name",
+                    function.name,
+                ],
+                capture_output=True,
+                check=True,
+            )

Likely invalid or redundant comment.

Tools
Ruff

356-356: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

sebs/knative/knative.py Outdated Show resolved Hide resolved
sebs/knative/knative.py Outdated Show resolved Hide resolved
sebs/knative/knative.py Outdated Show resolved Hide resolved
sebs/knative/knative.py Show resolved Hide resolved
sebs/knative/knative.py Outdated Show resolved Hide resolved
sebs/knative/knative.py Outdated Show resolved Hide resolved
sebs/knative/knative.py Outdated Show resolved Hide resolved
Signed-off-by: Abhishek Kumar <[email protected]>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 088ca51 and 574cbd7.

Files selected for processing (1)
  • tools/knative_preparation.py (1 hunks)
Additional comments not posted (5)
tools/knative_preparation.py (5)

15-16: LGTM!

The function is simple and correctly checks if a command is available in the system's PATH.


18-24: LGTM! Ensure run_command has error handling.

The function correctly installs Minikube if it is not already installed. Ensure run_command has proper error handling as suggested.


26-34: LGTM! Ensure run_command has error handling.

The function correctly installs kubectl if it is not already installed. Ensure run_command has proper error handling as suggested.


36-43: LGTM! Ensure run_command has error handling.

The function correctly installs Cosign if it is not already installed. Ensure run_command has proper error handling as suggested.


72-79: LGTM!

The main execution block correctly calls the installation functions in sequence.

tools/knative_preparation.py Outdated Show resolved Hide resolved
tools/knative_preparation.py Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 574cbd7 and b803caf.

Files selected for processing (18)
  • benchmarks/000.microbenchmarks/010.sleep/input.py (1 hunks)
  • benchmarks/000.microbenchmarks/020.network-benchmark/input.py (1 hunks)
  • benchmarks/000.microbenchmarks/030.clock-synchronization/input.py (1 hunks)
  • benchmarks/000.microbenchmarks/040.server-reply/input.py (1 hunks)
  • benchmarks/wrappers/knative/python/func.py (1 hunks)
  • config/example.json (1 hunks)
  • config/systems.json (1 hunks)
  • install.py (3 hunks)
  • requirements.knative.txt (1 hunks)
  • sebs/config.py (2 hunks)
  • sebs/knative/init.py (1 hunks)
  • sebs/knative/config.py (1 hunks)
  • sebs/knative/function.py (1 hunks)
  • sebs/knative/knative.py (1 hunks)
  • sebs/knative/triggers.py (1 hunks)
  • sebs/sebs.py (1 hunks)
  • sebs/utils.py (2 hunks)
  • tools/knative_preparation.py (1 hunks)
Files skipped from review due to trivial changes (3)
  • requirements.knative.txt
  • sebs/config.py
  • sebs/knative/init.py
Files skipped from review as they are similar to previous changes (4)
  • benchmarks/wrappers/knative/python/func.py
  • sebs/knative/function.py
  • sebs/knative/triggers.py
  • tools/knative_preparation.py
Additional context used
Ruff
sebs/knative/config.py

213-213: Local variable cached_config is assigned to but never used

Remove assignment to unused variable cached_config

(F841)

sebs/knative/knative.py

49-49: Do not use mutable data structures for argument defaults

Replace with None; initialize within function

(B006)


213-213: Local variable CONFIG_FILES is assigned to but never used

Remove assignment to unused variable CONFIG_FILES

(F841)


294-294: Local variable language is assigned to but never used

Remove assignment to unused variable language

(F841)


359-359: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


395-395: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


399-399: Local variable docker_image is assigned to but never used

Remove assignment to unused variable docker_image

(F841)


419-419: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


424-424: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


443-443: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

Additional comments not posted (20)
benchmarks/000.microbenchmarks/020.network-benchmark/input.py (1)

6-6: Ensure that the benchmarks_bucket parameter is utilized if needed.

The parameter benchmarks_bucket is added but not used within the function. Verify if this parameter is necessary or if it should be used within the function logic.

benchmarks/000.microbenchmarks/030.clock-synchronization/input.py (1)

6-6: Ensure that the benchmarks_bucket parameter is utilized if needed.

The parameter benchmarks_bucket is added but not used within the function. Verify if this parameter is necessary or if it should be used within the function logic.

benchmarks/000.microbenchmarks/010.sleep/input.py (1)

11-11: Ensure that the benchmarks_bucket parameter is utilized if needed.

The parameter benchmarks_bucket is added but not used within the function. Verify if this parameter is necessary or if it should be used within the function logic.

benchmarks/000.microbenchmarks/040.server-reply/input.py (1)

11-11: Ensure that the benchmarks_bucket parameter is utilized if needed.

The parameter benchmarks_bucket is added but not used within the function. Verify if this parameter is necessary or if it should be used within the function logic.

config/example.json (1)

91-109: Ensure correctness of Knative deployment configuration.

The Knative deployment configuration includes properties such as shutdownStorage, removeCluster, knativeExec, docker_registry, and storage. Ensure that these configurations are correctly implemented and consistent with other deployments.

Verification successful

Knative deployment configuration is correct and complete.

The Knative deployment configuration includes all the necessary properties (shutdownStorage, removeCluster, knativeExec, docker_registry, and storage) and their values are correctly set.

  • config/example.json: Lines 91-109
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correctness of Knative deployment configuration.

# Test: Check if the Knative configuration includes all the necessary properties and values.
rg --json --pretty 'knative' config/example.json

Length of output: 1064


Script:

#!/bin/bash
# Description: Verify the presence and correctness of all properties in the Knative deployment configuration.

# Check for the presence and values of all properties within the "knative" configuration.
rg '"knative": {' -A 20 config/example.json

Length of output: 133


Script:

#!/bin/bash
# Description: Verify the presence and correctness of all properties in the Knative deployment configuration.

# Check for the presence and values of all properties within the "knative" configuration.
rg '"knative": \{' -A 25 config/example.json

Length of output: 550

install.py (1)

66-71: Ensure correctness of Knative dependencies installation.

The new lines handle the installation of Knative-specific dependencies. Ensure that the installation commands are correct and consistent with the existing logic for other deployments.

config/systems.json (1)

237-272: Ensure completeness and correctness of Knative configuration.

The newly added lines handle the configuration for Knative, including base images, deployment files, and packages. Ensure that the configurations are complete, correct, and consistent with other systems.

Verification successful

Ensure completeness and correctness of Knative configuration.

The Knative configuration in config/systems.json includes the necessary properties and values for python and nodejs languages. It is consistent with the configurations of other systems like AWS, Azure, GCP, and OpenWhisk. Each configuration block includes base_images, images, username, and deployment (with files and packages), which aligns with the patterns observed in other systems.

  • Knative Configuration:

    • Python: Includes base_images, images, username, and deployment (with files and packages).
    • Nodejs: Includes base_images, images, username, and deployment (with files and packages).
  • Comparison with Other Systems:

    • All systems have similar structure and properties, ensuring consistency.

No issues were found with the Knative configuration. It appears complete and consistent with other system configurations.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the completeness and correctness of Knative configuration.

# Test: Check if the Knative configuration includes all the necessary properties and values.
rg --json --pretty 'knative' config/systems.json

Length of output: 840


Script:

#!/bin/bash
# Extract the entire Knative configuration block
ast-grep --lang json --pattern '{
  "knative": $_
}' config/systems.json

# Extract configuration blocks of other systems for comparison
ast-grep --lang json --pattern '{
  $_: {
    "languages": $_
  }
}' config/systems.json

Length of output: 623


Script:

#!/bin/bash
# Extract the entire Knative configuration block
ast-grep --lang json --pattern '{
  "knative": {
    "languages": $_
  }
}' config/systems.json

# Extract configuration blocks of other systems for comparison
ast-grep --lang json --pattern '{
  "languages": $_
}' config/systems.json

Length of output: 174


Script:

#!/bin/bash
# Extract and display context around the knative configuration for manual inspection
rg -A 50 'knative' config/systems.json

Length of output: 787


Script:

#!/bin/bash
# Extract and display configurations of other systems for comparison
rg -A 50 '"languages"' config/systems.json

Length of output: 5596

sebs/sebs.py (1)

107-112: Ensure correctness of Knative integration.

The newly added lines handle the integration of Knative as a deployment option. Ensure that the integration logic is correct and consistent with the existing logic for other deployments.

sebs/utils.py (1)

92-99: LGTM!

The function sanitize_benchmark_name_for_knative correctly sanitizes benchmark names for Knative.

sebs/knative/config.py (3)

9-27: LGTM!

The class KnativeCredentials correctly handles Docker credentials for Knative.


30-173: LGTM!

The class KnativeResources correctly handles Docker registry and Minio storage configurations for Knative.


176-227: LGTM!

The class KnativeConfig correctly handles the overall configuration for Knative, including resources and credentials.

Tools
Ruff

213-213: Local variable cached_config is assigned to but never used

Remove assignment to unused variable cached_config

(F841)

sebs/knative/knative.py (8)

24-48: LGTM!

The __init__ method correctly initializes the Knative object and handles Docker login if credentials are provided.


49-51: LGTM!

The initialize method correctly calls initialize_resources with an optional resource prefix.

Tools
Ruff

49-49: Do not use mutable data structures for argument defaults

Replace with None; initialize within function

(B006)


56-71: LGTM!

The get_storage method correctly retrieves or initializes Knative Minio storage based on the provided configuration.


73-80: LGTM!

The shutdown method correctly stops storage if required and deletes the Knative cluster if configured.


115-189: LGTM!

The build_base_image method correctly builds the base image for the function using the 'func build' command.


454-457: LGTM!

The cached_function method correctly sets logging handlers for HTTP triggers of a function.


458-462: LGTM!

The default_function_name method correctly generates a default function name based on the benchmark and language details.


361-396: Use capture_output instead of sending stdout and stderr to PIPE.

The capture_output argument should be used for better readability and maintainability.

-            subprocess.run(
-                [
-                    *self.get_knative_func_cmd(),
-                    "deploy",
-                    "--path",
-                    code_package.code_location,
-                ],
-                stderr=subprocess.PIPE,
-                stdout=subprocess.PIPE,
-                check=True,
-            )
+            subprocess.run(
+                [
+                    *self.get_knative_func_cmd(),
+                    "deploy",
+                    "--path",
+                    code_package.code_location,
+                ],
+                capture_output=True,
+                check=True,
+            )

Likely invalid or redundant comment.

Tools
Ruff

395-395: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

sebs/knative/knative.py Outdated Show resolved Hide resolved
sebs/knative/knative.py Outdated Show resolved Hide resolved
sebs/knative/knative.py Outdated Show resolved Hide resolved
sebs/knative/knative.py Outdated Show resolved Hide resolved
sebs/knative/knative.py Outdated Show resolved Hide resolved
sebs/knative/knative.py Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 42

Outside diff range, codebase verification and nitpick comments (4)
sebs/knative/config.py (1)

119-135: Improve logging for storage configuration updates.

Enhance the logging messages to provide more context about the storage configuration updates.

- ret.logging.info(
-    "User-provided configuration is different from cached storage, "
-    "we will update existing Knative function."
- )
+ ret.logging.info(
+    "User-provided storage configuration differs from cached storage. "
+    "Updating existing Knative function with new storage configuration."
+ )
docs/platforms.md (3)

339-345: Use consistent list style for prerequisites.

The list style for prerequisites should be consistent with the rest of the document. Replace dashes with asterisks.

- - `kubectl`
- - `kind`
- - `helm`
- - `jq`
- - `func` (It's a [Knative function CLI](https://knative.dev/docs/functions/install-func/#installing-the-func-cli) that is used to interact with your Knative functions directly from your command line. SeBS also makes use of this tool to `build` and `deploy` the benchmarks as Knative functions.)
- - `minio`
+ * `kubectl`
+ * `kind`
+ * `helm`
+ * `jq`
+ * `func` (It's a [Knative function CLI](https://knative.dev/docs/functions/install-func/#installing-the-func-cli) that is used to interact with your Knative functions directly from your command line. SeBS also makes use of this tool to `build` and `deploy` the benchmarks as Knative functions.)
+ * `minio`
Tools
Markdownlint

340-340: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


341-341: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


342-342: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


343-343: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


344-344: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


345-345: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


349-355: Clarify the path to kubeconfig.yaml.

The instruction to export the KUBECONFIG path should specify the exact location of the bin folder.

- export KUBECONFIG=path/to/bin/folder/kubeconfig.yaml
+ export KUBECONFIG=$(pwd)/bin/kubeconfig.yaml

386-387: Clarify the benchmark deployment example.

The example command should include a brief explanation of each argument.

- ./sebs.py benchmark invoke 311.compression test --config config/example.json --deployment knative --language python --language-version 3.9
+ # Example command to deploy a benchmark to the Knative-enabled cluster
+ ./sebs.py benchmark invoke 311.compression test \
+   --config config/example.json \
+   --deployment knative \
+   --language python \
+   --language-version 3.9
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between b803caf and 16f1d07.

Files selected for processing (13)
  • benchmarks/wrappers/knative/nodejs/index.js (1 hunks)
  • benchmarks/wrappers/knative/nodejs/storage.js (1 hunks)
  • benchmarks/wrappers/knative/python/func.py (1 hunks)
  • benchmarks/wrappers/knative/python/storage.py (1 hunks)
  • config/systems.json (1 hunks)
  • docs/platforms.md (1 hunks)
  • sebs/benchmark.py (1 hunks)
  • sebs/knative/config.py (1 hunks)
  • sebs/knative/function.py (1 hunks)
  • sebs/knative/knative.py (1 hunks)
  • sebs/knative/triggers.py (1 hunks)
  • sebs/regression.py (2 hunks)
  • tools/knative_setup.sh (1 hunks)
Files skipped from review as they are similar to previous changes (2)
  • config/systems.json
  • sebs/knative/function.py
Additional context used
Biome
benchmarks/wrappers/knative/nodejs/storage.js

[error] 52-52: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 53-53: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)


[error] 55-55: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

Ruff
benchmarks/wrappers/knative/python/func.py

5-5: flask.jsonify imported but unused

Remove unused import: flask.jsonify

(F401)


7-7: minio imported but unused

Remove unused import: minio

(F401)

benchmarks/wrappers/knative/python/storage.py

3-3: json imported but unused

Remove unused import: json

(F401)

sebs/knative/config.py

215-215: Local variable cached_config is assigned to but never used

Remove assignment to unused variable cached_config

(F841)

sebs/knative/knative.py

52-52: Do not use mutable data structures for argument defaults

Replace with None; initialize within function

(B006)


348-348: Local variable language is assigned to but never used

Remove assignment to unused variable language

(F841)


424-424: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


460-460: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


466-466: Local variable docker_image is assigned to but never used

Remove assignment to unused variable docker_image

(F841)


488-488: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


495-495: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


519-519: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

Shellcheck
tools/knative_setup.sh

[warning] 28-28: HELM appears unused. Verify use (or export if used externally).

(SC2034)


[warning] 29-29: JQ appears unused. Verify use (or export if used externally).

(SC2034)


[warning] 48-48: red appears unused. Verify use (or export if used externally).

(SC2034)


[warning] 54-54: grey appears unused. Verify use (or export if used externally).

(SC2034)


[warning] 56-56: yellow appears unused. Verify use (or export if used externally).

(SC2034)


[warning] 71-71: Declare and assign separately to avoid masking return values.

(SC2155)

Markdownlint
docs/platforms.md

340-340: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


341-341: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


342-342: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


343-343: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


344-344: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


345-345: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

Additional comments not posted (27)
benchmarks/wrappers/knative/python/storage.py (1)

48-51: Add error handling for upload method.

Consider adding error handling for the upload method to handle potential exceptions from the Minio client.

try:
  self.client.fput_object(bucket, key_name, filepath)
except Exception as e:
  logging.error(f"Failed to upload file {file} to bucket {bucket}: {e}")
  raise e

Likely invalid or redundant comment.

sebs/knative/config.py (3)

9-13: Ensure sensitive data is handled securely.

The KnativeCredentials class stores sensitive data like docker_username and docker_password. Ensure these are handled securely.
[security]


32-43: Ensure sensitive data is handled securely.

The KnativeResources class stores sensitive data like docker_username and docker_password. Ensure these are handled securely.
[security]


182-189: Ensure sensitive data is handled securely.

The KnativeConfig class stores sensitive data like knativeExec, shutdownStorage, and removeCluster. Ensure these are handled securely.
[security]

sebs/knative/knative.py (3)

119-193: Ensure build_base_image method handles errors and logs relevant information.

The method should handle potential errors and log relevant information during the Docker image build process.


530-533: Ensure cached_function method handles errors.

The cached_function method should handle potential errors during logging handler updates.


306-424: Fix undefined variable function_cfg.

The function_cfg variable is used before it is defined.

-                res = KnativeFunction(
-                    func_name, code_package.benchmark, code_package.hash, function_cfg
-                )
-                self.update_function(res, code_package)
+                function_cfg = KnativeFunctionConfig.from_benchmark(code_package)
+                function_cfg.storage = cast(KnativeMinio, self.get_storage()).config
+                function_cfg.url = function_url
+                res = KnativeFunction(
+                    func_name,
+                    code_package.benchmark,
+                    code_package.hash,
+                    function_cfg,
+                )
+                self.update_function(res, code_package)

Likely invalid or redundant comment.

Tools
Ruff

348-348: Local variable language is assigned to but never used

Remove assignment to unused variable language

(F841)


424-424: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

sebs/benchmark.py (1)

299-299: LGTM!

The change to append a newline character after each package in the "requirements.txt" file improves readability and structure.

tools/knative_setup.sh (16)

1-13: Header looks good!

The licensing information and brief description are correctly included.


19-23: Initialization function looks good!

The init function correctly initializes the necessary steps.


110-145: Main function looks good!

The main function correctly orchestrates the setup process.


101-108: Version setting function looks good!

The set_versions function correctly sets the versions for various components.


147-168: Function to get the latest release version looks good!

The get_latest_release_version function correctly retrieves the latest version from the given repository.


170-173: Function to check if the current branch is a release branch looks good!

The is_release_branch function correctly determines if the current branch is a release branch.


175-185: Function to get the current branch looks good!

The current_branch function correctly retrieves the current branch name.


187-218: Kubernetes setup function looks good!

The kubernetes function correctly sets up the Kubernetes cluster.


220-236: Knative Serving setup function looks good!

The serving function correctly sets up Knative Serving.


238-258: DNS configuration function looks good!

The dns function correctly configures DNS settings.


260-290: Load balancer setup function looks good!

The loadbalancer function correctly sets up the load balancer.


292-315: Networking setup function looks good!

The networking function correctly sets up the ingress controller.


317-331: Knative Eventing setup function looks good!

The eventing function correctly sets up Knative Eventing.


333-340: Namespace creation function looks good!

The namespace function correctly creates and labels the namespace.


342-368: Local registry setup function looks good!

The registry function correctly sets up the local registry.


370-392: Next steps function looks good!

The next_steps function correctly provides the next steps for managing the cluster.

sebs/regression.py (3)

292-309: KnativeTestSequencePython class looks good!

The class correctly extends unittest.TestCase and uses TestSequenceMeta as its metaclass. The get_deployment method is implemented correctly.


311-327: KnativeTestSequenceNodejs class looks good!

The class correctly extends unittest.TestCase and uses TestSequenceMeta as its metaclass. The get_deployment method is implemented correctly.


411-420: Regression suite function looks good!

The regression_suite function correctly includes the new Knative test sequences.

benchmarks/wrappers/knative/nodejs/index.js Show resolved Hide resolved
benchmarks/wrappers/knative/nodejs/index.js Outdated Show resolved Hide resolved
benchmarks/wrappers/knative/nodejs/index.js Show resolved Hide resolved
benchmarks/wrappers/knative/nodejs/index.js Outdated Show resolved Hide resolved
benchmarks/wrappers/knative/nodejs/index.js Show resolved Hide resolved
benchmarks/wrappers/knative/python/storage.py Show resolved Hide resolved
benchmarks/wrappers/knative/python/storage.py Show resolved Hide resolved
tools/knative_setup.sh Show resolved Hide resolved
tools/knative_setup.sh Show resolved Hide resolved
tools/knative_setup.sh Show resolved Hide resolved
@mcopik
Copy link
Collaborator

mcopik commented Jul 19, 2024

@coderabbitai pause

Copy link

coderabbitai bot commented Jul 19, 2024

Actions performed

Reviews paused.

benchmarks/wrappers/knative/nodejs/storage.js Show resolved Hide resolved
benchmarks/wrappers/knative/python/func.py Show resolved Hide resolved
config/example.json Show resolved Hide resolved
config/systems.json Outdated Show resolved Hide resolved
config/systems.json Outdated Show resolved Hide resolved
sebs/knative/knative.py Show resolved Hide resolved
sebs/knative/knative.py Outdated Show resolved Hide resolved
sebs/knative/knative.py Outdated Show resolved Hide resolved
sebs/utils.py Outdated Show resolved Hide resolved
tools/knative_setup.sh Show resolved Hide resolved
@octonawish-akcodes
Copy link
Collaborator Author

This current changes makes all the benchmarks run on knative platform with redeployments, Need final reviews here :)) @mcopik @oanarosca

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.

3 participants