Implemented new REST API hpFlows to add high priority flows to Azkaban. #3014

djaiswal83 · 2021-12-08T01:02:04Z

These high priority flows when run in containeirzed dispatch mode, will only use ACTIVE version of the docker image of all the jobtypes required.

This ensures that these flows always run on a stable version rather than any version which is being ramped up and is potentially unstable.
There API can be only called by either Azkaban admins or the designated owners.
Currently the ability to add owners is a manual process. An ADMIN only API to add such owners will come in the future.
API to remove a flow or list of flows from the high priority flows will also come in future.

Added unit tests as and where needed.

…ll only use ACTIVE version of the docker image of all the jobtypes required. This ensures that these flows always run on a stable version rather than any version which is being ramped up and is potentially unstable. There API can be only called by either Azkaban admins or the designated owners. Currently the ability to add owners is a manual process. An ADMIN only API to add such owners will come in the future. API to remove a flow or list of flows from the high priority flows will also come in future. Added unit tests as and where needed.

amielkelinkedin · 2022-01-03T19:33:18Z

azkaban-common/src/main/java/azkaban/imagemgmt/daos/HPFlowDaoImpl.java

+   */
+  @Override
+  public boolean isHPFlowOwner(final String userId) {
+    final FetchHPFlowOwnership fetchHPFlowOwnership = new FetchHPFlowOwnership();


Is it necessary to create a new object every time?

I am not sure, I believe I followed the pattern used in the rest of the code. Do you see any harm in doing this?

My understanding is that in general dependencies should be injected. I don't see a specific issue here, but I think it might be better memory management to only create the object as necessary, especially since it seems like we're using this in a mostly static context (only to access the encapsulated query).

amielkelinkedin · 2022-01-03T19:35:22Z

azkaban-common/src/main/java/azkaban/imagemgmt/daos/HPFlowDaoImpl.java

+      log.info("Added the HP flows by user " + userId);
+      return count;
+    } catch (final SQLException e) {
+      log.error("Unable to add HP flows", e);


Are we interested in adding the user in this log message, if we're adding the user in the info message?

That is a good catch. Will add.

amielkelinkedin · 2022-01-03T19:36:27Z

azkaban-common/src/main/java/azkaban/imagemgmt/daos/HPFlowDaoImpl.java

+      if (e.getErrorCode() == SQL_ERROR_CODE_DUPLICATE_ENTRY) {
+        errorMessage = "Reason: One or more flows already exists.";
+      } else {
+        errorMessage = "Reason: ";


Shall we say "Reason: Unknown" here, or something else?

amielkelinkedin · 2022-01-03T19:37:46Z

azkaban-common/src/main/java/azkaban/imagemgmt/dto/HPFlowDTO.java

+public class HPFlowDTO extends BaseDTO {
+  // CSV of flow ids.
+  @JsonProperty("flowIds")
+  @NotBlank(message = "flowList cannot be empty.")


Should this be flowIds?

Will do. Should be consistent.

amielkelinkedin · 2022-01-03T19:38:56Z

azkaban-common/src/main/java/azkaban/imagemgmt/dto/HPFlowDTO.java

+   * @return list of flow IDs.
+   */
+  public List<String> getFlowIdList() {
+    if (this.flowIds.isEmpty()) {


How is this possible without an error from the Not Blank annotation?

Its a sanity check. If you think its useless, let me know.

I think it would be better not to validate twice, but that is just my personal preference.

azkaban-db/src/main/sql/create.containerization-tables-all.sql

amielkelinkedin · 2022-01-03T19:47:15Z

azkaban-web-server/src/main/java/azkaban/imagemgmt/servlets/HPFlowServlet.java

+  }
+
+  @Override
+  protected void handleGet(final HttpServletRequest req,


OOC: Is this a todo or do we have no plans to implement the Get call?

At the time when this PR was created, expectation was to have basic functionality implemented hence this one was not implemented. However, things have now changed, it will be implemented in the upcoming commit for this PR.

amielkelinkedin · 2022-01-03T19:54:25Z

azkaban-web-server/src/main/java/azkaban/imagemgmt/servlets/HPFlowServlet.java

+   * @param user
+   * @return
+   */
+  private boolean hasHPFlowManagementPermission(final User user) {


I might be incorrect as I am going from memory, but wouldn't a direct call to the permission manager provide the same functionality?

This API checks if the user is Azakaban admin or not, if yes, then PermissionManager is not called.

You're correct. I was miss-remembering. Thank you.

jakhani

How will users be added as owners for hp_flows? Will it be manual task and db entry directly? Can you please also add ticket for getHpFlows? It will be useful API. It will also be good to show this information on /status page. Can you please add task for that too?

jakhani · 2022-01-05T06:35:53Z

azkaban-common/src/main/java/azkaban/imagemgmt/permission/PermissionManager.java

+   * @return
+   * @throws ImageMgmtException
+   */
+  public boolean hasPermission(final String userId) throws ImageMgmtException;


This method is validating permission for hp flow. Can you please rename this method to convey the same?

jakhani · 2022-01-05T06:37:32Z

azkaban-common/src/main/java/azkaban/imagemgmt/permission/PermissionManagerImpl.java

+    }
+
+    // Throw exception with FORBIDDEN error.
+    log.error(String.format("API access permission check failed. The user %s "


Can you please create object for error message and use it for both logging as well as exception?

Good point, will do.

jakhani · 2022-01-05T06:39:58Z

azkaban-common/src/main/java/azkaban/imagemgmt/daos/HPFlowDao.java

+ */
+public interface HPFlowDao {
+  /**
+   * isHPFlowOwner


Can you please appropriate comment for each method? For example, why is hpFlowOwner only taking userId and no reference for flow? Shouldn't there be a mapping between user and flow in metadata?
Please add appropriate method doc for rest of the methods too.

As stated in reply to main comment, the idea was to have only a handful of owners for a certain jobtype to use this functionality. The logic with mapping will come in next commit.

jakhani · 2022-01-05T06:42:26Z

azkaban-common/src/main/java/azkaban/imagemgmt/daos/HPFlowDaoImpl.java

+
+  // Flow ID is a fully qualified flow name with format,
+  // <project_name>.<flow_ame>
+  private static String INSERT_HP_FLOW_OWNER =


Is there any mapping between hp_flow_owners and hp_flows tables? Can anyone be owner for any flows and insert anything? Can you please add more detail about who can be the owner?

Replied in earlier comment.

jakhani · 2022-01-05T06:43:23Z

azkaban-common/src/main/java/azkaban/imagemgmt/daos/HPFlowDaoImpl.java

+    final FetchHPFlowOwnership fetchHPFlowOwnership = new FetchHPFlowOwnership();
+    try {
+      final String returnedName = this.databaseOperator
+          .query(FetchHPFlowOwnership.FETCH_HP_OWNER_BY_NAME,


Please rename FETCH_HP_OWNER_BY_NAME to FETCH_HP_FLOW_OWNER_BY_NAME?

Makes sense!

jakhani · 2022-01-05T07:08:24Z

azkaban-db/src/main/sql/create.containerization-tables-all.sql

+-- Definition for hp_flow_owners table. It contains list of owners who have
+-- access to add/remove high priority flows.
+CREATE TABLE IF NOT EXISTS hp_flow_owners (
+    owner            VARCHAR(64)     NOT NULL PRIMARY KEY,


I saw insert and select query where this column name is "name". This sql script and code in PR will break things. Can you please test this part and change column name appropriately?

Good catch. I tested it myself but I am admin so it never went to user query, will fix it.

Please test everything as user on solo-server and verify all the APIs. You will miss many bugs otherwise.

jakhani · 2022-01-05T07:08:48Z

azkaban-db/src/main/sql/create.containerization-tables-all.sql

+-- Definition for hp_flows. It contains the flow_id in format "project_name.flow_name".
+
+CREATE TABLE IF NOT EXISTS hp_flows (
+    flow_id           VARCHAR(256)   NOT NULL PRIMARY KEY,


Please rename this field based on my comments about flowId and flowName.

jakhani · 2022-01-05T07:11:28Z

azkaban-web-server/src/main/java/azkaban/imagemgmt/services/HPFlowServiceImpl.java

+
+  @VisibleForTesting
+  List<String> getFlowIdList(final HPFlowDTO hpFlowDTO) {
+    return hpFlowDTO.getFlowIdList();


What is the input to get FlowIdList? Why is it needed?

Please add appropriate scope for this method and documentation.

This method is for testing only, will add comments.

jakhani · 2022-01-05T07:12:44Z

azkaban-web-server/src/main/java/azkaban/imagemgmt/servlets/HPFlowServlet.java

+
+  private static final Logger log = Logger.getLogger(HPFlowServlet.class);
+  private static final String BASE_HP_FLOW_URI = "/hpFlows";
+  private ConverterUtils converterUtils;


Util should not be an instance. All the methods in utils should be public static. Can you please remove this instance?

+1 with @jakhani

It is not an instance, in the init() method, it is fetched from the server.

Utils should only have static methods.

jakhani · 2022-01-05T07:15:36Z

azkaban-db/src/main/sql/create.containerization-tables-all.sql

+-- Definition for hp_flows. It contains the flow_id in format "project_name.flow_name".
+
+CREATE TABLE IF NOT EXISTS hp_flows (
+    flow_id           VARCHAR(256)   NOT NULL PRIMARY KEY,


Please add index for flow_id if there is chance of it growing.

It is primary key, it is indexed by default.

hqin2016 · 2022-01-05T23:15:52Z

azkaban-db/src/main/sql/create.containerization-tables-all.sql

@@ -120,3 +120,20 @@ CREATE UNIQUE INDEX version_set_md5
 -- alter table execution_flows add column dispatch_method TINYINT default 1;
 -- CREATE INDEX ex_flows_dispatch_method ON execution_flows (dispatch_method);

+-- Definition for hp_flow_owners table. It contains list of owners who have


Is there a design doc for the db tables, dao and service?

I didn't see a method to update the hp_flow_owners table, such as insert or delete. Also it might be useful if we add a state (active 1 vs inactive 0) col for the owners table.

As earlier stated, the original intent of this PR was to get basic functionality out and this was supposed to be followed by implementation for ownership mapping. However, this did not go in time due to other factors and the rest of the implementation will be done in this PR only.

hqin2016 · 2022-01-05T23:21:50Z

azkaban-common/src/main/java/azkaban/imagemgmt/permission/PermissionManager.java

+   * @return
+   * @throws ImageMgmtException
+   */
+  public boolean hasPermission(final String userId) throws ImageMgmtException;


Method name should be hasHPFlowPermission

I kept it same as there is already a method with same name.

hqin2016 · 2022-01-05T23:24:02Z

azkaban-common/src/main/java/azkaban/imagemgmt/permission/PermissionManagerImpl.java

+   * @return boolean
+   */
+  @Override
+  public boolean hasPermission(final String userId) {


It seems all HP owners have permission to update the hp_flows table.

In this PR, yes. Eventually no.

hqin2016 · 2022-01-05T23:27:42Z

azkaban-db/src/main/sql/create.containerization-tables-all.sql

+-- Definition for hp_flows. It contains the flow_id in format "project_name.flow_name".
+
+CREATE TABLE IF NOT EXISTS hp_flows (
+    flow_id           VARCHAR(256)   NOT NULL PRIMARY KEY,


How about index col and project_id col?

flow_id is fully qualified with project name.

djaiswal83 · 2022-01-13T08:10:40Z

At this PR creation time, the idea was to keep handful of owners for a jobtype partner who will add the HP flows. However, now the priorities have changed and we can work on implementation to manage ownership mapping.

amielkelinkedin · 2022-01-13T20:42:10Z

azkaban-web-server/src/main/java/azkaban/imagemgmt/servlets/HPFlowServlet.java

+   */
+  private boolean hasHPFlowManagementPermission(final User user) {
+    /**
+     * Azkaban ADMIN role must have full permission to access image management APIs. Hence, no


Is this copy-paste text perhaps?

amielkelinkedin reviewed Jan 3, 2022

View reviewed changes

jakhani reviewed Jan 5, 2022

View reviewed changes

hqin2016 reviewed Jan 5, 2022

View reviewed changes

amielkelinkedin reviewed Jan 13, 2022

View reviewed changes

Implemented new REST API hpFlows to add high priority flows to Azkaban. #3014

Are you sure you want to change the base?

Implemented new REST API hpFlows to add high priority flows to Azkaban. #3014

Conversation

djaiswal83 commented Dec 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakhani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djaiswal83 Jan 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djaiswal83 Jan 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djaiswal83 commented Jan 13, 2022

Choose a reason for hiding this comment

djaiswal83 Jan 13, 2022 •

edited

djaiswal83 Jan 13, 2022 •

edited