Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/23411 )
Change subject: KUDU-1457 [4/n] enable webserver for IPv6 ...................................................................... Patch Set 5: (20 comments) Thank you, Alexey, for providing the feedback! http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/master/mini_master-test.cc File src/kudu/master/mini_master-test.cc: http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/master/mini_master-test.cc@40 PS5, Line 40: class MiniMasterTest : public KuduTest, : public ::testing::WithParamInterface<std::string> { : protected: : static std::string get_host() { : FLAGS_ip_config_mode = GetParam(); : if (FLAGS_ip_config_mode == "ipv6") { : return "::1"; : } : return "127.0.0.1"; : } : }; : : // This is used to run all parameterized tests with different IP modes. : INSTANTIATE_TEST_SUITE_P(Parameters, MiniMasterTest, : testing::Values("ipv4", "ipv6")); > Shouldn't there be coverage for the 'dual' mode here as well? Same reason here as well. If a test had covered two distinct cases i.e. 1. Using loopback address i.e. 127.0.0.1 2. Using unspecified address i.e. 0.0.0.0 In such a test, it would make sense to have two more addresses covered as well i.e. "::1" for ipv6 mode and "::" for dual mode. There is no reason that these tests won't run for dual mode unless there is some specific ip based communication happening that considers just the ip address we set here. Given similar conditions on a node, if a bind to ::1 works, chances are highly likely that bind to :: works as well. If you still feel that dual mode needs to be covered, I don't mind adding. http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/master/mini_master.cc File src/kudu/master/mini_master.cc: http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/master/mini_master.cc@70 PS5, Line 70: "[" + web_bind_addr.host() + "]" > Shouldn't this be somehow encapsulated into HostPort or just a stand-alone This is just one time usage here. Even though similar sort manipulation is being done in Restart() below but that relies on ip family saved in bound_http_ object member rather than a flag. I guess both could simply rely on the flag i.e. FLAGS_ip_config_mode. Let me see if that is possible. http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc File src/kudu/server/webserver-test.cc: http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@188 PS5, Line 188: "ipv4", "dual" > Why 'ipv6' is omitted? Could you at least add a comment about the reason? The idea is to have a parallel and corresponding test for an existing scenario in ipv6/dual mode. For example, if a test has requirement to bind to a any/unspecified ip address (0.0.0.0), it would have a corresponding test in dual mode that binds with IN6ADDR_ANY_INIT i.e. :: which is its equivalent in IPv6. Similarly, if there is a test where loopback address (127.0.0.1) is used for binding, it would have equivalent as ::1 in IPv6. In theory, we could have all three modes for all the tests, but it would not serve much purpose. If there was need to have a test run for both unspecified address or loopback address, it would have been done in IPv4 as well but it isn't. Tests using WebserverAdvertisedAddressesTest have specified addresses, so two modes are applicable i.e. ipv4 and ipv6. Similarly, for rest of the tests, unspecified address is being used so ipv4 and dual modes become applicable. Theoretically, we could add all three modes but that may not add much value because the idea is to ensure webserver is initialised to a given address in specific scenarios and that is being achieved by running those test for mix of all three modes as applicable. http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@288 PS5, Line 288: "ipv4", "dual" > ditto regarding the omitted 'ipv6' ditto explanation from above http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@299 PS5, Line 299: "ipv4", "dual" > ditto regarding the omitted 'ipv6' ditto explanation from above http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@425 PS5, Line 425: ipv4", "dual > ditto regarding the omitted 'ipv6' ditto explanation from above http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@526 PS5, Line 526: pv4", "dual > ditto regarding the omitted 'ipv6' ditto explanation from above http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@539 PS5, Line 539: ipv4", "dual > ditto regarding the omitted 'ipv6' ditto explanation from above http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@744 PS5, Line 744: ipv4", "dual > ditto regarding the omitted 'ipv6' ditto explanation from above http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@753 PS5, Line 753: ipv4", "dual > ditto regarding the omitted 'ipv6' ditto explanation from above http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@830 PS5, Line 830: "ipv4", "dual") > ditto regarding the omitted 'ipv6' ditto explanation from above http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@893 PS5, Line 893: "ipv4", "dual" > ditto regarding the omitted 'ipv6' ditto explanation from above http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@994 PS5, Line 994: "127.0.0.1" > nit: why not to use expected_advertised_host_ here as well? Done http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@1017 PS5, Line 1017: CHECK(false) > nit: would calling FAIL() and returning an empty string fit here, or you ex I am not really keen on crashing with SIGABRT. Does FAIL() have any variant that returns a string? I couldn't find any. http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver_options.cc File src/kudu/server/webserver_options.cc: http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver_options.cc@47 PS5, Line 47: 0.0.0.0 > Does this implicitly assume IPv4 then? This aligns with --rpc_bind_addresses that is also set to default 'unspecified' address. The default mode for ip_config_mode is IPv4 so bind addresses (for both rpc and webserver) is also default to IPv4 unspecified address. http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/util/net/net_util.cc@88 PS5, Line 88: Value is case-sensitive. > Why? Being case-sensitive it's not user-friendly, and many of our string f That's a type. Fixed now. http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/util/net/net_util.cc@115 PS5, Line 115: Status ParseIPModeFlag(const string& name, > Why to have this as a standalone function if it's only used in ValidateIPCo Done http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/util/net/net_util.cc@124 PS5, Line 124: "IP config mode cannot be any value other than 'ipv4' " : "or 'ipv6' or 'dual'. Options are case in-sensitive." > nit: replace with Done http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/util/net/net_util.cc@365 PS5, Line 365: FLAGS_ip_config_mode == "ipv4" > Here and elsewhere where this flag is involved: since the value for the fla That is a miss from my side. I think I wanted to use helper function but somehow missed doing that here. Marton also suggested something similar about enum usage. I guess now that value is case insensitive, the need to have enum based approach makes sense. 'family' has default value set to AF_UNSPEC. Maybe remove the need of 'family' altogether and simply rely on the flag is something that can be done. Just like it is done in GetFQDN. I will check and get back on this. http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/util/net/net_util.cc@372 PS5, Line 372: hints.ai_family = family > Is this consistent if FLAGS_ip_config_mode == 'dual'? Yes, the default value for family is AF_UNSPEC which is applicable to dual mode. -- To view, visit http://gerrit.cloudera.org:8080/23411 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7095561539f427b5c58ada7b480ca549b0ab795e Gerrit-Change-Number: 23411 Gerrit-PatchSet: 5 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Tue, 30 Sep 2025 13:51:47 +0000 Gerrit-HasComments: Yes
