glassfish
  1. glassfish
  2. GLASSFISH-15289

Regression in create instance and a better error message needed

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Cannot Reproduce
    • Affects Version/s: 3.1_b33
    • Fix Version/s: 4.1
    • Component/s: admin
    • Labels:
      None
    • Environment:

      build: ogs-3.1-b34-12_20_2010.zip

      Description

      In Admin Console create a CONFIG type node filling out the required fields only. Go to Standalone Instances page and click new. Attempt to create an instance using the newly created node and the following error is displayed:

      An error has occurred
      Node conf-node does not have the nodehost attribute set and therefore create-instance can not be used with this node. You must either add the nodehost attribute using the update-node-config command, or create the instance by running create-local-instance directly on the instance host.

      The above used to work in earlier builds. If this regression cannot be fixed, we should at least tailor the error message towards the Admin Console users, since that's where it is displayed and tell them how to fix the problem in Admin Console alone (CLI information can be present additionally, if desired).

        Activity

        Hide
        carlavmott added a comment -

        The nodehost is required and will continue to be. There are scenarios where the nodehost is required and there is no way to make this option required in some cases and optional in others so it will be required.

        Not sure what to do about messages. It is the cli command that is running (from the console) so the message is specific for cli users. If we tailor the message for console then it doesn't make sense for cli users. This is a problem that affects several messages in the cli commands.

        Show
        carlavmott added a comment - The nodehost is required and will continue to be. There are scenarios where the nodehost is required and there is no way to make this option required in some cases and optional in others so it will be required. Not sure what to do about messages. It is the cli command that is running (from the console) so the message is specific for cli users. If we tailor the message for console then it doesn't make sense for cli users. This is a problem that affects several messages in the cli commands.
        Hide
        carlavmott added a comment -

        this is not a regression and nodehost will be required.

        Assigning to Anissa because the cli should not change the message so it is suitable for the console. The console will have to come up with a suitable message.

        Show
        carlavmott added a comment - this is not a regression and nodehost will be required. Assigning to Anissa because the cli should not change the message so it is suitable for the console. The console will have to come up with a suitable message.
        Hide
        Anissa Lam added a comment -

        GUI cannot analysis what error message says and thus there is no way for GUI to 'replace' or 'add' any info to display to user.
        It is a general issue that backend is treating GUI user as second class citizen, and have message geared towards CLI only. This is not the only case. We may have to live with this for 3.1.

        As for whether the 'nodeHost' should be a required field in GUI or not, this has been changing back and forth couple times. The last I heard from Joe is we should NOT require that since it will affect other user case.
        Here is what the email says:

        Email from Joe on 12/20/2010 9:20AM says:

        Anissa Lam wrote:
        >
        > Didn't hear anything from Joe about this since the GUI changed after the meeting.
        > Joe, please confirm the required fields for CONFIG node.

        There are two use cases. One use case does not require the fields. The
        second use case does.

        If you are going to create the instance with create-local-instance
        the fields are not required – in fact it is best if the user does
        not enter them.

        If you are going to do offline config with create-instance then the
        fields are required (nodehost and installdir). At this time I'm not
        comfortable removing the requirement on nodehost. Installdir is
        possible – not sure of the consequences of removing the check for
        that. At this point I rather just live with needing the two fields
        if you are doing offline config.

        I would not change the GUI to require them since that impacts
        the first use case.

        Joe

        ======================================

        So, no more change in GUI
        As for the error message, transfer back to Carla. If she can add something like "or go to Edit Node screen in GUI to add the required info", that will be good.
        If she doesn't want to do this, she can either close as won't fix or defer to 3.2.

        Show
        Anissa Lam added a comment - GUI cannot analysis what error message says and thus there is no way for GUI to 'replace' or 'add' any info to display to user. It is a general issue that backend is treating GUI user as second class citizen, and have message geared towards CLI only. This is not the only case. We may have to live with this for 3.1. As for whether the 'nodeHost' should be a required field in GUI or not, this has been changing back and forth couple times. The last I heard from Joe is we should NOT require that since it will affect other user case. Here is what the email says: Email from Joe on 12/20/2010 9:20AM says: Anissa Lam wrote: > > Didn't hear anything from Joe about this since the GUI changed after the meeting. > Joe, please confirm the required fields for CONFIG node. There are two use cases. One use case does not require the fields. The second use case does. If you are going to create the instance with create-local-instance the fields are not required – in fact it is best if the user does not enter them. If you are going to do offline config with create-instance then the fields are required (nodehost and installdir). At this time I'm not comfortable removing the requirement on nodehost. Installdir is possible – not sure of the consequences of removing the check for that. At this point I rather just live with needing the two fields if you are doing offline config. I would not change the GUI to require them since that impacts the first use case. Joe ====================================== So, no more change in GUI As for the error message, transfer back to Carla. If she can add something like "or go to Edit Node screen in GUI to add the required info", that will be good. If she doesn't want to do this, she can either close as won't fix or defer to 3.2.
        Hide
        Anissa Lam added a comment -

        btw, GUI screen also matches what CLI required:

        Usage: asadmin [asadmin-utility-options] create-node-config
        [--nodedir <nodedir>] [--nodehost <nodehost>]
        [--installdir <installdir>] [?|-help[=<help(default:false)>]] name

        Show
        Anissa Lam added a comment - btw, GUI screen also matches what CLI required: Usage: asadmin [asadmin-utility-options] create-node-config [--nodedir <nodedir>] [--nodehost <nodehost>] [--installdir <installdir>] [ ?| -help [=<help(default:false)>] ] name
        Hide
        carlavmott added a comment -

        Taking the issues separately.

        nodehost is required for the offline case and I believe that what Joe is saying is that he is not comfortable changing that. So nodehost will also be required. The GUI and the cli stay as they are. I believe this part of the issue is closed in that no change will be made.

        That leaves the message part. If the admin gui does not change the message or can not then I will suggest that we do not change it for the cli since there are several places this message and similar messages are created and our only option is to create a message that is very generic. So it will not tell the user to run a command and it will not tell them to go to a specific screen. I think it is too late to try and fix the messages since testing involves making sure we hit all the error cases and in the pat this has taken a lot of time. I will defer til 3.2

        Show
        carlavmott added a comment - Taking the issues separately. nodehost is required for the offline case and I believe that what Joe is saying is that he is not comfortable changing that. So nodehost will also be required. The GUI and the cli stay as they are. I believe this part of the issue is closed in that no change will be made. That leaves the message part. If the admin gui does not change the message or can not then I will suggest that we do not change it for the cli since there are several places this message and similar messages are created and our only option is to create a message that is very generic. So it will not tell the user to run a command and it will not tell them to go to a specific screen. I think it is too late to try and fix the messages since testing involves making sure we hit all the error cases and in the pat this has taken a lot of time. I will defer til 3.2
        Hide
        Anissa Lam added a comment -

        Joe said:
        "I would not change the GUI to require them since that impacts the first use case. "
        so, GUI doesn't require them and that is what we have now. (as of today 12/21).

        However, when i save a CONFIG node, I am getting the error:
        "nodehost attribute is required to update node dummy-config". (refer to attached update-config-error.jpg.)

        and when i use CLI, I get error as well.

        %v3admin update-node-config dummy-config
        org.glassfish.api.admin.CommandException: remote failure: nodehost attribute is required to update node dummy-config.
        Command update-node-config failed.

        So, backend is enforcing the requirement of nodehost when both CLI and GUI is telling user this is optional.

        You need to remove the requirement of nodehost. otherwise, both CLI and GUI user runs into error everytime.

        I am removing the 3.1_exclude tag. Please at least fix this part.

        Show
        Anissa Lam added a comment - Joe said: "I would not change the GUI to require them since that impacts the first use case. " so, GUI doesn't require them and that is what we have now. (as of today 12/21). However, when i save a CONFIG node, I am getting the error: "nodehost attribute is required to update node dummy-config". (refer to attached update-config-error.jpg.) and when i use CLI, I get error as well. %v3admin update-node-config dummy-config org.glassfish.api.admin.CommandException: remote failure: nodehost attribute is required to update node dummy-config. Command update-node-config failed. So, backend is enforcing the requirement of nodehost when both CLI and GUI is telling user this is optional. You need to remove the requirement of nodehost. otherwise, both CLI and GUI user runs into error everytime. I am removing the 3.1_exclude tag. Please at least fix this part.
        Hide
        lidiam added a comment -

        There is another scenario with fields that are not marked as required but need to be specified when creating an instance. I create a CONFIG node specifying localhost for host name and leaving other fields blank. I then attempt to create an instance under such node from Admin Console but it fails due to the following error:

        An error has occurred
        Node conf-node does not have the installdir attribute set and therefore create-instance can not be used with this node. You must either add the installdir attribute using the update-node-config command, or create the instance by running create-local-instance directly on the instance host.

        This will be confusing to users. I hope we can default installation directory for CONFIG node if the host is localhost. Please let me know if this should be logged as a separate issue.

        This used to work in earlier builds, when Installation Directory was defaulted for CONFIG nodes.

        Show
        lidiam added a comment - There is another scenario with fields that are not marked as required but need to be specified when creating an instance. I create a CONFIG node specifying localhost for host name and leaving other fields blank. I then attempt to create an instance under such node from Admin Console but it fails due to the following error: An error has occurred Node conf-node does not have the installdir attribute set and therefore create-instance can not be used with this node. You must either add the installdir attribute using the update-node-config command, or create the instance by running create-local-instance directly on the instance host. This will be confusing to users. I hope we can default installation directory for CONFIG node if the host is localhost. Please let me know if this should be logged as a separate issue. This used to work in earlier builds, when Installation Directory was defaulted for CONFIG nodes.
        Hide
        carlavmott added a comment -

        Yes, things have changed and some changes were a result of Arch review and some were because validation was added for the offline scenario case.

        I take Joe's response to mean that we are not changing the back end because it is too risky but he doesn't see an issue with leaving the GUI screen as it is. Right now both nodehost and installdir are required but the underlying code even though in some cases it really is not necessary because we can figure out the value.

        I'm going to exclude this bug from 3.1 because we will not change the backend code.

        Show
        carlavmott added a comment - Yes, things have changed and some changes were a result of Arch review and some were because validation was added for the offline scenario case. I take Joe's response to mean that we are not changing the back end because it is too risky but he doesn't see an issue with leaving the GUI screen as it is. Right now both nodehost and installdir are required but the underlying code even though in some cases it really is not necessary because we can figure out the value. I'm going to exclude this bug from 3.1 because we will not change the backend code.
        Hide
        carlavmott added a comment -

        The scenario in this test was really not handled in 3.1. For 3.2 release I have added support to allow create-instance to create the instance config element when the node element doesn't have the nodehost or the installdir specified. The user will have to run create-local-instance to create the instance file system and that will also update the node element with the nodehost and installdir information. All other references to that node will also inherit the nodehost and installdir.

        This scenario is closest to what was supported in GlassFish v2 where the node agent provided the information at instance creation time if it didn't exist. See scenario 8 on the Instance Lifecycle Scenarios wiki page http://wikis.sun.com/display/GlassFish/3.1SSHScenarios

        Show
        carlavmott added a comment - The scenario in this test was really not handled in 3.1. For 3.2 release I have added support to allow create-instance to create the instance config element when the node element doesn't have the nodehost or the installdir specified. The user will have to run create-local-instance to create the instance file system and that will also update the node element with the nodehost and installdir information. All other references to that node will also inherit the nodehost and installdir. This scenario is closest to what was supported in GlassFish v2 where the node agent provided the information at instance creation time if it didn't exist. See scenario 8 on the Instance Lifecycle Scenarios wiki page http://wikis.sun.com/display/GlassFish/3.1SSHScenarios
        Hide
        carlavmott added a comment -

        nodehost and installdir no longer required when creating a config node

        Committed revision 44848.

        Still need to work on error message

        Show
        carlavmott added a comment - nodehost and installdir no longer required when creating a config node Committed revision 44848. Still need to work on error message
        Hide
        carlavmott added a comment -

        O plan to fix this for EE7 release. The only thing to do is to update the error message to include information on how to proceed in the GUI and don't plan on removing the instructions for the CLI. This fix will impact testing since they will need to verify.

        Show
        carlavmott added a comment - O plan to fix this for EE7 release. The only thing to do is to update the error message to include information on how to proceed in the GUI and don't plan on removing the instructions for the CLI. This fix will impact testing since they will need to verify.
        Hide
        carlavmott added a comment -

        I can not recreate this error condition as behavior has changed and the error message that was in question is no longer printed. Therefore I have nothing to change.

        Show
        carlavmott added a comment - I can not recreate this error condition as behavior has changed and the error message that was in question is no longer printed. Therefore I have nothing to change.

          People

          • Assignee:
            carlavmott
            Reporter:
            lidiam
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: